Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweaked Confirmation Prompt and Added Test #2253

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Tweaked Confirmation Prompt and Added Test #2253

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9260c3
Added confirmation prompt for late days
Nita242004 Dec 8, 2024
92490d9
Merge branch 'master' into handin-confirmation-prompt
Nita242004 Dec 8, 2024
61f50a7
modify message and add testing
Dec 9, 2024
fd3cf02
edit message
Dec 9, 2024
e10db9d
change expected message
Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions app/views/assessments/_handin_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
<% days_late = (days_late / 1.day).ceil %>
<% grace_days_left = @aud.grace_days_usable %>
<% if grace_days_left >= days_late %>
<% grace_late_info = "submitting #{pluralize(days_late, 'day')} late using #{pluralize(days_late, 'grace day')}" %>
<% grace_late_info = "#{pluralize(days_late, 'day')} late using #{pluralize(days_late, 'grace day')}" %>
<% elsif grace_days_left > 0 %>
<% grace_late_info = "submitting #{pluralize(days_late, 'day')} late using #{pluralize(grace_days_left, 'grace day')} with #{pluralize(days_late - grace_days_left, 'penalty late day')}" %>
<% grace_late_info = "#{pluralize(days_late, 'day')} late using #{pluralize(grace_days_left, 'grace day')} and #{pluralize(days_late - grace_days_left, 'penalty late day')}" %>
<% else %>
<% grace_late_info = "submitting #{pluralize(days_late, 'day')} late using #{pluralize(days_late, 'penalty late day')}" %>
<% grace_late_info = "#{pluralize(days_late, 'day')} late using #{pluralize(days_late, 'penalty late day')}" %>
<% end %>
<% late_confirm = " and that I am #{grace_late_info} under the late policy as defined in the syllabus" %>
<p><b>Warning:</b> Submitting late may result in the usage of a grace day or a grade penalty!</p>
<% late_confirm = " and are #{grace_late_info}" %>
<p><b>Warning:</b> Submitting late will result in grace day usage or penalties as per the syllabus!</p>
<% end %>

<div class="row">
Expand Down Expand Up @@ -44,20 +44,20 @@
<% end %>
<%= label_tag(:integrity_checkbox) do %>
<%= check_box_tag(:integrity_checkbox) %>
<%= content_tag("span", "I affirm that I have complied with this course's academic
integrity policy as defined in the syllabus#{late_confirm}.") %>
<%= content_tag("span", "I affirm that I comply with this course's academic
integrity policy#{late_confirm}.") %>
<% 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!" }) %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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" and aria-modal="true" to the confirmation dialog
  • Include an aria-labelledby attribute 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

</div>
<% else %>
<div class="col s6 m4 center-align">
<%= f.submit("Submit", id: "fake-submit", class: "btn primary handin-btn disabled") %>
<%= f.submit("Submit", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Autolab Notification: Confirm submission?" }) %>
</div>
<% end %>
</div>
Expand Down
19 changes: 19 additions & 0 deletions spec/models/handin_form_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'rails_helper'

RSpec.describe "handin_form", type: :view do
let(:assessment) { create(:assessment) }
let(:aud) { create(:assessment_user_datum, assessment: assessment, past_due_at: true) }
let(:grace_late_info) { "using 1 late day" }

before do
assign(:aud, aud)
allow(view).to receive(:grace_late_info).and_return(grace_late_info)
render partial: "assessments/handin_form", locals: { f: ActionView::Helpers::FormBuilder.new(nil, nil, self, {}) }
end

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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