Feature/Add Staff Grant Extension endpoint #56

SahiruWithanage · 2025-04-06T09:41:57Z

Description

This pull request implements the Staff Grant Extension backend feature for OnTrack.

It introduces a new API endpoint that allows staff members to grant extensions to students directly — even when no formal extension request exists. This supports special circumstances and improves flexibility in managing assessments.

The work also involved refactoring the existing student extension flow (extension_comments_api.rb) to use the same shared service (ExtensionService). Now, both staff- and student-initiated extension requests are handled using the same logic, ensuring consistency and reducing duplication.

Related project: Staff Grant Extension (Design + Requirements Documentation)

⚠️ This PR has been rebased onto the latest 9.x branch.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactor (internal restructuring without changing behavior)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (update or new)

How Has This Been Tested?

The following tests have been written and run to verify correct behavior:

Unit tests in staff_grant_extension_test.rb
Verified that the service logic correctly mirrors the existing student flow
Confirmed that both staff and student flows now use ExtensionService
Confirmed that only authorized staff can access the endpoint
Tested handling of valid and invalid data

Note: This is backend-only work. Frontend integration is tracked separately.

Checklist

If involving code

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code in hard-to-understand areas
My changes generate no new warnings

If modified config files

Folders and Files Added/Modified

Added:

app/api/staff_grant_extension_api.rb
app/services/extension_service.rb
test/api/staff_grant_extension_test.rb

Modified:

app/api/api_root.rb
app/models/unit.rb
app/api/extension_comments_api.rb

b0ink · 2025-04-06T19:01:05Z

Looks good to me, i was able to run your code and verify your tests worked using rails test

b0ink

One suggestion i have is to replace any double quotes with single quotes that isn't using any string interpolation - if you're not embedding any variables/data into your string, use single quotes

b0ink · 2025-04-06T19:05:02Z

test/api/staff_grant_extension_test.rb

+    assert response["successful"].length == 1, "Should have one successful extension"
+    assert response["failed"].empty?, "Should have no failed extensions"
+    assert response["successful"][0]["student_id"] == project.student.id, "Should match the student ID"
+    assert response["successful"][0]["weeks_requested"] == 1, "Should have requested 1 week"
+    assert response["successful"][0]["extension_response"].present?, "Should have extension response"
+    assert response["successful"][0]["task_status"].present?, "Should have task status"


single quotes instead of double quotes

SahiruWithanage · 2025-04-11T09:01:30Z

@b0ink Thanks for the suggestion! I went ahead and applied the single quote change you mentioned — really appreciated. Also ended up refactoring extension_comments_api.rb to use the shared service, so both student and staff flows are now aligned.

Note: You can use the test file for the extension_comments_api.rb to test the endpoint. Its called extension_test.rb made by Mr. Andrew.

JoeMacl

This looks good to me! Code is really clean and easy to follow. Just a few suggestions.

You could use [weeks_requested, max_duration].min for the duration calculation just to make it a bit cleaner.
Maybe rename result to extension in the success response for clarity since it's returning the extension object.

Apart from that this is looking solid! Nice work! :)

samindiii

Hello Sahiru! You’ve done a great job! The code is clean, well-structured, and the functionality seems to work in the way that we want :)

Well done on the separation of staff/student checks and creating that common logic so that the original functionality is still in place!

I have suggested some changes, they're really minor but feel free to let me know if you have any questions. Again, these are just suggestions-your code works the way it is intended to!

samindiii · 2025-04-14T10:31:25Z

app/services/extension_service.rb

+    duration = max_duration unless weeks_requested <= max_duration
+
+    # Check if extension would exceed deadline
+    return { success: false, error: 'Extensions cannot be granted beyond task deadline', status: 403 } if duration <= 0


This sounds a little confusing. It's really saying the requested extension is invalid, but the wording is about a deadline check.

Maybe replace with something like "Extension weeks must be greater than zero"

Hey! Appreciate the suggestion! After checking out the code, I think the error message "Extensions cannot be granted beyond task deadline" is way more on point than "Extension weeks must be greater than zero." The first one really captures the business rule we're trying to enforce. When the duration dips to 0 or goes negative, it’s usually because the task is already late or the extension would push it beyond the deadline. So, the current message does a solid job of letting users know about that deadline issue. Therefore, I recommend keeping the current message as it provides more meaningful feedback to users.

samindiii · 2025-04-14T10:36:26Z

app/services/extension_service.rb

+    # Check if extension would exceed deadline
+    return { success: false, error: 'Extensions cannot be granted beyond task deadline', status: 403 } if duration <= 0
+
+    # ===== Student Request Logic (current endpoint) =====


Also very minor but changing the comment "Student Request Logic" to something like "Student-Initiated Extension Logic" might help with understanding!

Changes have been made, will push the code soon! Thank you.

samindiii · 2025-04-14T10:43:18Z

app/api/staff_grant_extension_api.rb

+        else
+          results[:failed] << {
+            student_id: student_id,
+            project_id: project.id,


Might be good to add a safe navigation here (project&.id) just in case grant_extension somehow fails and another exception is raised.

Hey! I believe adding safe navigation (&.) to project.id might not be necessary here because if project were nil, we would have already encountered an error earlier when using it for grant_extension (task.rb). The project object is obtained through a database query or relationship that would raise an error if the project doesn't exist. Adding safe navigation at this point would mask potential issues that should be caught and handled earlier in the process. Therefore, I recommend keeping the current implementation.

samindiii · 2025-04-14T10:49:19Z

app/services/extension_service.rb

+    # Calculate max duration
+    max_duration = task.weeks_can_extend
+    duration = weeks_requested
+    duration = max_duration unless weeks_requested <= max_duration


If the weeks-requested is higher than the max_duration, this line will set the extension duration to be max_duration. This is good but it might be good to have an error message or something like that to inform the user that there's been a change in the extension duration they requested.

Maybe something like this??
if weeks_requested > task.weeks_can_extend
return { success: false, error: "You can only request up to #{task.weeks_can_extend} week(s)", status: 403 }
end

Thank you for the suggestion. The current behavior of silently adjusting the extension duration to the maximum allowed is actually intentional - it's a logic that was established for student-initiated extensions. When implementing the staff grant extension feature, we maintained this same behavior for consistency. The system automatically adjusts the duration to the maximum allowed while still processing the extension, which has been working well in the student context. Therefore, I recommend keeping this existing behavior rather than adding the error message, as it maintains consistency with how student-initiated extensions are handled before the extension service was added.

b0ink · 2025-05-05T00:33:41Z

Looks good to me, you have made the correct changes switching to single quotes and I was able to confirm again your code is working using your tests via rails test

aNebula

LGTM!
@SahiruWithanage Please open an upstream PR with these changes against doubtfire-lms/doubtfire-api 9.x branch.

Please rebase your changes on the 9.x branch.

Enable staff to grant extensions to multiple students without formal requests. Reuse existing student extension logic through a new service for consistency. Supports flexible academic support and streamlines staff workflows. Relates to the OnTrack Staff Grant Extension design documentation.

…rpolated strings This aligns the test file with the string formatting convention used in the rest of the codebase. Single quotes are preferred when string interpolation is not needed, improving consistency. Reviewed as part of peer feedback.

Linked extension_comments_api (student-requested extensions) to use the shared ExtensionService, previously set up for staff-granted extensions. This refactor ensures both student and staff extension flows use the same logic, improving consistency and reducing duplication.

SahiruWithanage · 2025-05-11T04:55:59Z

⚠️ Update: This PR has been rebased onto the latest 9.x branch.

aNebula · 2025-06-17T12:49:29Z

LGTM!
@SahiruWithanage Please open an upstream PR with these changes against doubtfire-lms/doubtfire-api 9.x branch.

SahiruWithanage marked this pull request as ready for review April 6, 2025 09:42

SahiruWithanage changed the title ~~feat(api): add staff grant extension endpoint~~ Feature(api): add staff grant extension endpoint Apr 6, 2025

SahiruWithanage changed the title ~~Feature(api): add staff grant extension endpoint~~ Feature/Add Staff Grant Extension endpoint Apr 6, 2025

b0ink reviewed Apr 6, 2025

View reviewed changes

aNebula changed the base branch from development to 9.x April 7, 2025 07:14

JoeMacl approved these changes Apr 12, 2025

View reviewed changes

samindiii reviewed Apr 14, 2025

View reviewed changes

samindiii approved these changes Apr 20, 2025

View reviewed changes

SahiruWithanage mentioned this pull request Apr 28, 2025

Feature/Extension notification system - Backend #60

Open

15 tasks

aNebula approved these changes May 6, 2025

View reviewed changes

Sahiru Withanage and others added 3 commits May 11, 2025 14:05

SahiruWithanage force-pushed the feature/staff-grant-extension branch from d95f8af to e11a841 Compare May 11, 2025 04:24

Change suggested comments changes.

66d8bf7

JoeMacl mentioned this pull request May 14, 2025

Feature/grant extension form v3 thoth-tech/doubtfire-web#330

Open

9 tasks

Feature/Add Staff Grant Extension endpoint #56

Are you sure you want to change the base?

Feature/Add Staff Grant Extension endpoint #56

Uh oh!

Conversation

SahiruWithanage commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist

If involving code

If modified config files

Folders and Files Added/Modified

Uh oh!

b0ink commented Apr 6, 2025

Uh oh!

b0ink left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SahiruWithanage commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeMacl left a comment

Choose a reason for hiding this comment

Uh oh!

samindiii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b0ink commented May 5, 2025

Uh oh!

aNebula left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SahiruWithanage commented May 11, 2025

Uh oh!

aNebula commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SahiruWithanage commented Apr 6, 2025 •

edited

Loading

SahiruWithanage commented Apr 11, 2025 •

edited

Loading

aNebula left a comment •

edited

Loading