Conversation
Add two API endpoints for fixing assignment due dates
…issing allow_extended_requests
- Fix double blank line in request.rb eligible_for_auto_approval? - Fix missing newline at EOF in enrollments.feature - Add specs for auto_approval_eligible_for_course? with extended days (zero standard days but positive extended days, both zero) - Add specs for eligible_for_auto_approval? edge cases: boundary (exactly max days), both days zero, no enrollment record fallback, max_auto_approve limit with extended requests - Add controller spec for non-existent enrollment (RecordNotFound) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing tests and fixes for auto extended requests
|
Dear professor @armandofox and professor @cycomachead, Basim and I have finished the feature and this passed all the tests both in cucumber and Rspecs. Your review is appreciated. Thank you! Anthony |
Filter Extension Requests by Student from Enrollments Page
cycomachead
left a comment
There was a problem hiding this comment.
Some followups.
This looks great.
Two main things, which I'd suggest doing on the same branch, then opening a new PR to your local repo.
- I think we'll need to add a little tooltip to the table row header to indicate what the setting means
- when the AJAX request is successful, I think we need to have a flash message (in green) that says "Enabled extended automatic approvals for [student name]"
- Or for failures "Failed to enable ..." (in red).
the flash messages could be a new PR if you'd like. I think they'd need to be done in JS and we'd probably want an additional test to ensure the text is shown (at least for success)
| def toggle_allow_extended_requests | ||
| @enrollment = @course.user_to_courses.find(params[:id]) | ||
|
|
||
| unless @role == 'instructor' |
There was a problem hiding this comment.
Checks like this seem a little brittle / unclear in the authorization.
Does it work to use the ensure_instructor_role method as a before_action?
Or I see the issue might be needing to return JSON?
If it needs to be its own method, I would extract this logic to a before_action that just lives in this controller for now. And if so, I think it'd be worth writing @enrollment.course_admin? or .staff? -- in this app we generally don't want only instructors to make changes.
|
|
||
| unless @role == 'instructor' | ||
| Rails.logger.error "Role #{@role} does not have permission to toggle allow_extended_requests" | ||
| flash.now[:alert] = 'You do not have permission to perform this action.' |
There was a problem hiding this comment.
Can we add "You must be an instructor or Lead TA." if we're going to use the course_admin? method?
| const allowExtended = checkbox.checked; | ||
|
|
||
| try { | ||
| const token = document.querySelector('meta[name="csrf-token"]')?.content || ''; |
There was a problem hiding this comment.
Fine for now, though if we do more AJAX-y things, I think we should write some fetchWithToken method or something that shared this logic.
| auto_approve(lms_facade_from_user) | ||
| end | ||
|
|
||
| def auto_approval_eligible_for_course? |
There was a problem hiding this comment.
Realizing this method totally belongs somewhere else, but we can leave it for now.
app/models/request.rb
Outdated
| def auto_approval_eligible_for_course? | ||
| return false if course&.course_settings.blank? | ||
|
|
||
| course.course_settings.enable_extensions && |
There was a problem hiding this comment.
| course.course_settings.enable_extensions && | |
| course.course_settings.automatic_approval_enabled? |
Let's go ahead and definite on course_settings
def automatic_approval_enabled?
return false unless enable_extensions?
auto_approve_days.positive? || auto_approve_extended_request_days.positive?
end
I realized I was actually wrong about not needing () in class... because yeah, there's a weird case:
irb(main):010:0> false && false || true
=> true
irb(main):011:0> false && (false || true)
=> false
irb(main):012:0>
So rather than need () we can also just move the method and use 2 or 3 lines and that's just more clear.
| return false if course&.course_settings.blank? | ||
| return false unless course.course_settings.enable_extensions? | ||
| return false if course.course_settings.auto_approve_days.zero? | ||
| return false if course.course_settings.auto_approve_days.zero? && course.course_settings.auto_approve_extended_request_days.zero? |
There was a problem hiding this comment.
hmm. if we fix the above method, can't we just write return false unless course_settings.automatic_approval_enabled?
Though, better yet, since we do have the above method defined,
I might write:
def eligible_for_auto_approval?
return false unless auto_approval_eligible_for_course?
...
I thought there should be a rubocop rule for this, but always leave a blank line between a guard clause a the next line of real code.
| <% end %> | ||
| </span> | ||
| <% if enrollment.role.downcase == 'student' %> | ||
| <% if enrollment.student? %> |
There was a problem hiding this comment.
Thanks for the update! :)
| <th class="text-center">Student ID</th> | ||
| <th class="text-center">Email</th> | ||
| <th class="text-center">Role</th> | ||
| <th class="text-center">Approved Extended?</th> |
There was a problem hiding this comment.
| <th class="text-center">Approved Extended?</th> | |
| <th class="text-center"> | |
| Extended Requests? | |
| <a tabindex="0" | |
| role="button" | |
| data-bs-toggle="popover" | |
| data-bs-trigger="focus" | |
| data-bs-html="true" | |
| data-bs-content="Allow students to have an additional window for automatically approved requests, such as for extenuating circumstances. Set the limit from <a href='<%= course_settings_path %>'>the settings page</a>." | |
| aria-label="Help: additional request window settings"> | |
| <i class="fas fa-circle-question" aria-hidden="true"></i> | |
| </a> | |
| </th> |
Can you see if this works (and reformat, or just reject this suggestion)
This is me using Claude + Bootstrap docs to generate the HTML
https://claude.ai/share/f1d9e7eb-52ca-4ff9-b387-76b246eea153
I didn't test, but it matched the smell test. I'd like a a little '?' in a circle next to the text in the head cell.
| <td class="text-center"><%= enrollment.user.student_id %></td> | ||
| <td class="text-center"><%= enrollment.user.email %></td> | ||
| <td class="text-center"><%= enrollment.role.downcase.capitalize %></td> | ||
| <td class="justify-content-center align-content-center"> |
There was a problem hiding this comment.
| <td class="justify-content-center align-content-center"> | |
| <td | |
| class="justify-content-center align-content-center" | |
| data-order="<%= enrollment.allow_extended_requests ? 1 : 0 %>" | |
| > |
This should allow the table to be sortable in data tables.
We will need to update the row's data-order value after the JSON request returns
| throw new Error(data.error || 'Error updating enrollment'); | ||
| } | ||
|
|
||
| console.log(`Enrollment ${enrollmentId} allow_extended_requests: ${allowExtended}`); |
There was a problem hiding this comment.
Let's not leave this in the production app. :)
There was a problem hiding this comment.
Instead can we please add create a flash message, but maybe that's a followup.
General Info
Client wants a feature where instructors can enable extended automatic extensions for students with extenuating circumstances. The feature has been divided into three user stories to be accomplished effectively:
UI (add a “Approve extended requests?” switch from instructor console) cs169/flextensions#309
Update student’s backend database when instructor toggles “Approve extended requests?" cs169/flextensions#310
When a DSP student requests an extension, it should be approved automatically based on extended accommodations cs169/flextensions#311
Changes
Adds ability to automatically approve extended requests from subset of students and corresponding UI
Testing
I added Rspec and cucumber tests for instructors' actions (toggling the button should update the database)
I added Rspec tests to make sure it works from the DSP students side.
Documentation
There's a existing textbox that describes the input form that instructors use to set the auto extended days.