-
Notifications
You must be signed in to change notification settings - Fork 129
Feature/Add Staff Grant Extension endpoint #56
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.
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
base: 9.x
Are you sure you want to change the base?
Changes from all commits
7b5a001
b60c1b3
e11a841
66d8bf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| require 'grape' | ||
|
|
||
| # | ||
| # API endpoint for staff to grant extensions to multiple students at once | ||
| # | ||
| class StaffGrantExtensionApi < Grape::API | ||
| helpers AuthenticationHelpers | ||
| helpers AuthorisationHelpers | ||
| helpers DbHelpers | ||
|
|
||
| before do | ||
| authenticated? | ||
| error!({ | ||
| error: 'Not authorized to grant extensions', | ||
| code: 'UNAUTHORIZED', | ||
| details: {} | ||
| }, 403) unless current_user.has_tutor_capability? | ||
| end | ||
|
|
||
| desc 'Grant extensions to multiple students', | ||
| detail: 'This endpoint allows staff to grant extensions to multiple students at once for a specific task. The operation is atomic - either all extensions are granted or none are. Students not found in the unit are automatically skipped without affecting the transaction.', | ||
| success: [ | ||
| { code: 201, message: 'Extensions granted successfully' } | ||
| ], | ||
| failure: [ | ||
| { code: 400, message: 'Some extensions failed to be granted' }, | ||
| { code: 403, message: 'Not authorized to grant extensions for this unit' }, | ||
| { code: 404, message: 'Unit or task definition not found' }, | ||
| { code: 500, message: 'Internal server error' } | ||
| ], | ||
| response: { | ||
| successful: [ | ||
| { | ||
| student_id: 'Integer - ID of the student', | ||
| project_id: 'Integer - ID of the project', | ||
| weeks_requested: 'Integer - Number of weeks extension granted', | ||
| extension_response: 'String - Human readable message with new due date', | ||
| task_status: 'String - Updated status of the task' | ||
| } | ||
| ], | ||
| failed: [ | ||
| { | ||
| student_id: 'Integer - ID of the student', | ||
| project_id: 'Integer - ID of the project', | ||
| error: 'String - Error message explaining why extension failed' | ||
| } | ||
| ], | ||
| skipped: [ | ||
| { | ||
| student_id: 'Integer - ID of the student', | ||
| reason: 'String - Reason why the student was skipped' | ||
| } | ||
| ] | ||
| } | ||
| params do | ||
| requires :student_ids, type: Array[Integer], desc: 'List of student IDs to grant extensions to' | ||
| requires :task_definition_id, type: Integer, desc: 'Task definition ID' | ||
| requires :weeks_requested, type: Integer, desc: 'Number of weeks to extend by' | ||
| requires :comment, type: String, desc: 'Reason for extension' | ||
| end | ||
| post '/units/:unit_id/staff-grant-extension' do | ||
| unit = Unit.find(params[:unit_id]) | ||
| task_definition = unit.task_definitions.find(params[:task_definition_id]) | ||
|
|
||
| # Use transaction to ensure atomic operation | ||
| ActiveRecord::Base.transaction do | ||
| results = { | ||
| successful: [], | ||
| failed: [], | ||
| skipped: [] | ||
| } | ||
|
|
||
| params[:student_ids].each do |student_id| | ||
| # Find project for this student in the unit | ||
| project = unit.projects.find_by(user_id: student_id) | ||
| if project.nil? | ||
| results[:skipped] << { | ||
| student_id: student_id, | ||
| reason: 'Student not found in unit' | ||
| } | ||
| next | ||
| end | ||
|
|
||
| result = ExtensionService.grant_extension( | ||
| project.id, | ||
| task_definition.id, | ||
| current_user, | ||
| params[:weeks_requested], | ||
| params[:comment], | ||
| true # is_staff_grant = true | ||
| ) | ||
|
|
||
| if result[:success] | ||
| extension_comment = result[:result] | ||
| results[:successful] << { | ||
| student_id: student_id, | ||
| project_id: project.id, | ||
| weeks_requested: extension_comment.extension_weeks, | ||
| extension_response: extension_comment.extension_response, | ||
| task_status: extension_comment.task.status | ||
| } | ||
| else | ||
| results[:failed] << { | ||
| student_id: student_id, | ||
| project_id: project.id, | ||
| error: result[:error] | ||
| } | ||
| # If it's a validation error (403), raise it immediately | ||
| error!({ error: result[:error] }, result[:status]) if result[:status] == 403 | ||
| end | ||
| end | ||
|
|
||
| # If any extensions failed (but not due to validation), rollback the entire transaction | ||
| if results[:failed].any? | ||
| error!({ error: 'Some extensions failed to be granted', results: results }, 400) | ||
| end | ||
|
|
||
| status 201 | ||
| present results, with: Grape::Presenters::Presenter | ||
| end | ||
| rescue ActiveRecord::RecordNotFound | ||
| error!({ error: 'Unit or task definition not found' }, 404) | ||
| rescue StandardError | ||
| error!({ error: 'An unexpected error occurred' }, 500) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| class ExtensionService | ||
| def self.grant_extension(project_id, task_definition_id, user, weeks_requested, comment, is_staff_grant = false) | ||
| # Find project and task | ||
| project = Project.find(project_id) | ||
| task_definition = project.unit.task_definitions.find(task_definition_id) | ||
| task = project.task_for_task_definition(task_definition) | ||
|
|
||
| # ===== Common Validation Logic (used by both endpoints) ===== | ||
| # Validate extension weeks | ||
| return { success: false, error: 'Extension weeks cannot be 0', status: 403 } if weeks_requested == 0 | ||
|
|
||
| # Calculate max duration | ||
| max_duration = task.weeks_can_extend | ||
| duration = weeks_requested | ||
| duration = max_duration unless weeks_requested <= max_duration | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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??
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| # Check if extension would exceed deadline | ||
| return { success: false, error: 'Extensions cannot be granted beyond task deadline', status: 403 } if duration <= 0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| # ===== Student-Initiated Extension Logic (current endpoint) ===== | ||
| unless is_staff_grant | ||
| # Check task-level authorization for student requests with specific permission hash | ||
| unless AuthorisationHelpers.authorise?(user, task, :request_extension, ->(role, perm_hash, other) { task.specific_permission_hash(role, perm_hash, other) }) | ||
| return { success: false, error: 'Not authorised to request an extension for this task', status: 403 } | ||
| end | ||
| end | ||
|
|
||
| # ===== Staff Grant Logic (new endpoint) ===== | ||
| if is_staff_grant | ||
| # Check unit-level authorization for staff grants | ||
| unless AuthorisationHelpers.authorise?(user, project.unit, :grant_extensions) | ||
| return { success: false, error: 'Not authorised to grant extensions for this unit', status: 403 } | ||
| end | ||
| end | ||
|
|
||
| # ===== Common Extension Logic ===== | ||
| # Apply the extension | ||
| result = task.apply_for_extension(user, comment, duration) | ||
|
|
||
| # Auto-approve if it's a staff grant | ||
| if is_staff_grant | ||
| extension_comment = result.becomes(ExtensionComment) | ||
| extension_comment.assess_extension(user, true, true) | ||
| end | ||
|
|
||
| { success: true, result: result, status: 201 } | ||
| rescue ActiveRecord::RecordNotFound => e | ||
| { success: false, error: 'Task or project not found', status: 404 } | ||
| rescue StandardError => e | ||
| { success: false, error: e.message, status: 500 } | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to add a safe navigation here (project&.id) just in case grant_extension somehow fails and another exception is raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! I believe adding safe navigation (
&.) toproject.idmight not be necessary here because ifprojectwere nil, we would have already encountered an error earlier when using it forgrant_extension(task.rb). Theprojectobject 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.