From 7b5a001ba70e52cf423f8cfd8400a5f8d161c382 Mon Sep 17 00:00:00 2001 From: Sahiru Withanage Date: Sun, 6 Apr 2025 18:54:24 +1000 Subject: [PATCH 1/4] feat(api): add staff grant extension endpoint 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. --- app/api/api_root.rb | 2 + app/api/staff_grant_extension_api.rb | 126 +++++++++++++ app/models/unit.rb | 3 +- app/services/extension_service.rb | 52 ++++++ test/api/staff_grant_extension_test.rb | 249 +++++++++++++++++++++++++ 5 files changed, 431 insertions(+), 1 deletion(-) create mode 100644 app/api/staff_grant_extension_api.rb create mode 100644 app/services/extension_service.rb create mode 100644 test/api/staff_grant_extension_test.rb diff --git a/app/api/api_root.rb b/app/api/api_root.rb index bde5f0236..09e9f0787 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -60,6 +60,7 @@ class ApiRoot < Grape::API mount LearningAlignmentApi mount ProjectsApi mount SettingsApi + mount StaffGrantExtensionApi mount StudentsApi mount Submission::PortfolioApi mount Submission::PortfolioEvidenceApi @@ -100,6 +101,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to LearningOutcomesApi AuthenticationHelpers.add_auth_to LearningAlignmentApi AuthenticationHelpers.add_auth_to ProjectsApi + AuthenticationHelpers.add_auth_to StaffGrantExtensionApi AuthenticationHelpers.add_auth_to StudentsApi AuthenticationHelpers.add_auth_to Submission::PortfolioApi AuthenticationHelpers.add_auth_to Submission::PortfolioEvidenceApi diff --git a/app/api/staff_grant_extension_api.rb b/app/api/staff_grant_extension_api.rb new file mode 100644 index 000000000..9c50aa48b --- /dev/null +++ b/app/api/staff_grant_extension_api.rb @@ -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 diff --git a/app/models/unit.rb b/app/models/unit.rb index 175e62c79..9fe9631d9 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -28,7 +28,8 @@ def self.permissions :download_stats, :download_unit_csv, :download_grades, - :exceed_capacity + :exceed_capacity, + :grant_extensions ] # What can convenors do with units? diff --git a/app/services/extension_service.rb b/app/services/extension_service.rb new file mode 100644 index 000000000..59cc7c4a2 --- /dev/null +++ b/app/services/extension_service.rb @@ -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 + + # 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) ===== + unless is_staff_grant + # Check task-level authorization for student requests + unless AuthorisationHelpers.authorise?(user, task, :request_extension) + 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 diff --git a/test/api/staff_grant_extension_test.rb b/test/api/staff_grant_extension_test.rb new file mode 100644 index 000000000..ad48c474f --- /dev/null +++ b/test/api/staff_grant_extension_test.rb @@ -0,0 +1,249 @@ +require 'test_helper' + +class StaffGrantExtensionTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def test_staff_grant_extension_success + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + + td = TaskDefinition.new({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Staff Grant Extension Test', + description: 'Test task for staff grant extension', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 1.week, + target_date: Time.zone.now + 1.week, + due_date: Time.zone.now + 2.weeks, + abbreviation: 'STAFFGRANTTEST', + restrict_status_updates: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + td.save! + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: td.id, + weeks_requested: 1, + comment: "Staff granted extension" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 201, last_response.status + + response = last_response_body + 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" + + td.destroy! + unit.destroy! + end + + def test_staff_grant_extension_unauthorized + unit = FactoryBot.create(:unit) + project = unit.projects.first + student = project.student # Using student instead of staff + td = unit.task_definitions.first + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: td.id, + weeks_requested: 1, + comment: "Unauthorized attempt" + } + + add_auth_header_for user: student + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 403, last_response.status, "Should not allow non-staff to grant extensions" + end + + def test_staff_grant_extension_invalid_weeks + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + td = unit.task_definitions.first + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: td.id, + weeks_requested: 0, + comment: "Invalid weeks" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 403, last_response.status, "Should not allow 0 weeks extension" + end + + def test_staff_grant_extension_negative_weeks + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + td = unit.task_definitions.first + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: td.id, + weeks_requested: -1, + comment: "Negative weeks" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 403, last_response.status, "Should not allow negative weeks extension" + end + + def test_staff_grant_extension_missing_params + unit = FactoryBot.create(:unit) + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + + data_to_post = { + student_ids: [1], + # Missing task_definition_id and weeks_requested + comment: "Missing params" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 400, last_response.status, "Should require all parameters" + end + + def test_staff_grant_extension_transaction_rollback + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + td = unit.task_definitions.first + + # Test case 1: One valid student, one skipped student + data_to_post = { + student_ids: [project.student.id, 999999], # One valid, one invalid + task_definition_id: td.id, + weeks_requested: 1, + comment: "Transaction test with skipped student" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 201, last_response.status, "Should succeed for valid student" + + response = last_response_body + assert response["successful"].length == 1, "Should have one successful extension" + assert response["skipped"].length == 1, "Should have one skipped student" + assert response["failed"].empty?, "Should have no failed extensions" + assert response["skipped"][0]["student_id"] == 999999, "Should have skipped the invalid student ID" + assert response["skipped"][0]["reason"] == "Student not found in unit", "Should have correct skip reason" + + # Verify only the valid student got an extension + task = project.task_for_task_definition(td) + assert task.extensions == 1, "Should have one extension for the valid student" + + # Test case 2: Test actual transaction rollback + # Create a second project to test with + project2 = unit.projects.create!( + user: FactoryBot.create(:user, role: Role.student), + enrolled: true + ) + + # First, grant extensions to both students + data_to_post = { + student_ids: [project.student.id, project2.student.id], + task_definition_id: td.id, + weeks_requested: 1, + comment: "Initial extensions" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 201, last_response.status, "Should succeed for both students" + + # Verify both students got extensions + task1 = project.task_for_task_definition(td) + task2 = project2.task_for_task_definition(td) + assert task1.extensions == 2, "First student should have two extensions" + assert task2.extensions == 1, "Second student should have one extension" + + # Now try to grant extensions with a task that would cause a failure + # Use a task that's past its deadline to force a failure + td.due_date = Time.zone.now - 1.day + td.save! + + data_to_post = { + student_ids: [project.student.id, project2.student.id], + task_definition_id: td.id, + weeks_requested: 1, + comment: "Transaction rollback test" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 403, last_response.status, "Should fail with 403 when task is past deadline" + + # Verify neither student got a new extension (transaction rolled back) + task1.reload + task2.reload + assert task1.extensions == 2, "First student should still have two extensions" + assert task2.extensions == 1, "Second student should still have one extension" + + td.destroy! + unit.destroy! + end + + def test_staff_grant_extension_invalid_unit + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + td = unit.task_definitions.first + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: td.id, + weeks_requested: 1, + comment: "Invalid unit" + } + + add_auth_header_for user: staff + post_json "/api/units/999999/staff-grant-extension", data_to_post + assert_equal 404, last_response.status, "Should return 404 for invalid unit" + end + + def test_staff_grant_extension_invalid_task + unit = FactoryBot.create(:unit) + project = unit.projects.first + staff = FactoryBot.create(:user, role: Role.tutor) + unit.employ_staff(staff, Role.tutor) + + data_to_post = { + student_ids: [project.student.id], + task_definition_id: 999999, + weeks_requested: 1, + comment: "Invalid task" + } + + add_auth_header_for user: staff + post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post + assert_equal 404, last_response.status, "Should return 404 for invalid task definition" + end +end From b60c1b3d7fb937d46f07b2fb08a1b62570b0e575 Mon Sep 17 00:00:00 2001 From: SahiruWithanage Date: Thu, 10 Apr 2025 02:43:54 +1000 Subject: [PATCH 2/4] refactor(tests): replace double quotes with single quotes in non-interpolated 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. --- test/api/staff_grant_extension_test.rb | 70 +++++++++++++------------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/test/api/staff_grant_extension_test.rb b/test/api/staff_grant_extension_test.rb index ad48c474f..f03b9a62e 100644 --- a/test/api/staff_grant_extension_test.rb +++ b/test/api/staff_grant_extension_test.rb @@ -38,7 +38,7 @@ def test_staff_grant_extension_success student_ids: [project.student.id], task_definition_id: td.id, weeks_requested: 1, - comment: "Staff granted extension" + comment: 'Staff granted extension' } add_auth_header_for user: staff @@ -46,12 +46,12 @@ def test_staff_grant_extension_success assert_equal 201, last_response.status response = last_response_body - 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" + 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' td.destroy! unit.destroy! @@ -67,12 +67,12 @@ def test_staff_grant_extension_unauthorized student_ids: [project.student.id], task_definition_id: td.id, weeks_requested: 1, - comment: "Unauthorized attempt" + comment: 'Unauthorized attempt' } add_auth_header_for user: student post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 403, last_response.status, "Should not allow non-staff to grant extensions" + assert_equal 403, last_response.status, 'Should not allow non-staff to grant extensions' end def test_staff_grant_extension_invalid_weeks @@ -86,12 +86,12 @@ def test_staff_grant_extension_invalid_weeks student_ids: [project.student.id], task_definition_id: td.id, weeks_requested: 0, - comment: "Invalid weeks" + comment: 'Invalid weeks' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 403, last_response.status, "Should not allow 0 weeks extension" + assert_equal 403, last_response.status, 'Should not allow 0 weeks extension' end def test_staff_grant_extension_negative_weeks @@ -105,12 +105,12 @@ def test_staff_grant_extension_negative_weeks student_ids: [project.student.id], task_definition_id: td.id, weeks_requested: -1, - comment: "Negative weeks" + comment: 'Negative weeks' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 403, last_response.status, "Should not allow negative weeks extension" + assert_equal 403, last_response.status, 'Should not allow negative weeks extension' end def test_staff_grant_extension_missing_params @@ -121,12 +121,12 @@ def test_staff_grant_extension_missing_params data_to_post = { student_ids: [1], # Missing task_definition_id and weeks_requested - comment: "Missing params" + comment: 'Missing params' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 400, last_response.status, "Should require all parameters" + assert_equal 400, last_response.status, 'Should require all parameters' end def test_staff_grant_extension_transaction_rollback @@ -141,23 +141,23 @@ def test_staff_grant_extension_transaction_rollback student_ids: [project.student.id, 999999], # One valid, one invalid task_definition_id: td.id, weeks_requested: 1, - comment: "Transaction test with skipped student" + comment: 'Transaction test with skipped student' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 201, last_response.status, "Should succeed for valid student" + assert_equal 201, last_response.status, 'Should succeed for valid student' response = last_response_body - assert response["successful"].length == 1, "Should have one successful extension" - assert response["skipped"].length == 1, "Should have one skipped student" - assert response["failed"].empty?, "Should have no failed extensions" - assert response["skipped"][0]["student_id"] == 999999, "Should have skipped the invalid student ID" - assert response["skipped"][0]["reason"] == "Student not found in unit", "Should have correct skip reason" + assert response["successful"].length == 1, 'Should have one successful extension' + assert response["skipped"].length == 1, 'Should have one skipped student' + assert response["failed"].empty?, 'Should have no failed extensions' + assert response["skipped"][0]["student_id"] == 999999, 'Should have skipped the invalid student ID' + assert response["skipped"][0]["reason"] == 'Student not found in unit', 'Should have correct skip reason' # Verify only the valid student got an extension task = project.task_for_task_definition(td) - assert task.extensions == 1, "Should have one extension for the valid student" + assert task.extensions == 1, 'Should have one extension for the valid student' # Test case 2: Test actual transaction rollback # Create a second project to test with @@ -171,18 +171,18 @@ def test_staff_grant_extension_transaction_rollback student_ids: [project.student.id, project2.student.id], task_definition_id: td.id, weeks_requested: 1, - comment: "Initial extensions" + comment: 'Initial extensions' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 201, last_response.status, "Should succeed for both students" + assert_equal 201, last_response.status, 'Should succeed for both students' # Verify both students got extensions task1 = project.task_for_task_definition(td) task2 = project2.task_for_task_definition(td) - assert task1.extensions == 2, "First student should have two extensions" - assert task2.extensions == 1, "Second student should have one extension" + assert task1.extensions == 2, 'First student should have two extensions' + assert task2.extensions == 1, 'Second student should have one extension' # Now try to grant extensions with a task that would cause a failure # Use a task that's past its deadline to force a failure @@ -193,18 +193,18 @@ def test_staff_grant_extension_transaction_rollback student_ids: [project.student.id, project2.student.id], task_definition_id: td.id, weeks_requested: 1, - comment: "Transaction rollback test" + comment: 'Transaction rollback test' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 403, last_response.status, "Should fail with 403 when task is past deadline" + assert_equal 403, last_response.status, 'Should fail with 403 when task is past deadline' # Verify neither student got a new extension (transaction rolled back) task1.reload task2.reload - assert task1.extensions == 2, "First student should still have two extensions" - assert task2.extensions == 1, "Second student should still have one extension" + assert task1.extensions == 2, 'First student should still have two extensions' + assert task2.extensions == 1, 'Second student should still have one extension' td.destroy! unit.destroy! @@ -221,12 +221,12 @@ def test_staff_grant_extension_invalid_unit student_ids: [project.student.id], task_definition_id: td.id, weeks_requested: 1, - comment: "Invalid unit" + comment: 'Invalid unit' } add_auth_header_for user: staff post_json "/api/units/999999/staff-grant-extension", data_to_post - assert_equal 404, last_response.status, "Should return 404 for invalid unit" + assert_equal 404, last_response.status, 'Should return 404 for invalid unit' end def test_staff_grant_extension_invalid_task @@ -239,11 +239,11 @@ def test_staff_grant_extension_invalid_task student_ids: [project.student.id], task_definition_id: 999999, weeks_requested: 1, - comment: "Invalid task" + comment: 'Invalid task' } add_auth_header_for user: staff post_json "/api/units/#{unit.id}/staff-grant-extension", data_to_post - assert_equal 404, last_response.status, "Should return 404 for invalid task definition" + assert_equal 404, last_response.status, 'Should return 404 for invalid task definition' end end From e11a841ee1c6d28760687c97d523dabefd3256a3 Mon Sep 17 00:00:00 2001 From: SahiruWithanage Date: Fri, 11 Apr 2025 18:47:06 +1000 Subject: [PATCH 3/4] refactor(api): unify extension handling via shared service 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. --- app/api/extension_comments_api.rb | 32 ++++++++++++++----------------- app/services/extension_service.rb | 4 ++-- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/app/api/extension_comments_api.rb b/app/api/extension_comments_api.rb index 3bde9171d..966d69e6f 100644 --- a/app/api/extension_comments_api.rb +++ b/app/api/extension_comments_api.rb @@ -10,25 +10,21 @@ class ExtensionCommentsApi < Grape::API requires :weeks_requested, type: Integer, desc: 'The details of the request' end post '/projects/:project_id/task_def_id/:task_definition_id/request_extension' do - project = Project.find(params[:project_id]) - task_definition = project.unit.task_definitions.find(params[:task_definition_id]) - task = project.task_for_task_definition(task_definition) - - # check permissions using specific permission has with addition of request extension if allowed in unit - unless authorise? current_user, task, :request_extension, ->(role, perm_hash, other) { task.specific_permission_hash(role, perm_hash, other) } - error!({ error: 'Not authorised to request an extension for this task' }, 403) + # Use the ExtensionService to handle the extension request + result = ExtensionService.grant_extension( + params[:project_id], + params[:task_definition_id], + current_user, + params[:weeks_requested], + params[:comment] + ) + + # Handle the service response + if result[:success] + present result[:result].serialize(current_user), Grape::Presenters::Presenter + else + error!({ error: result[:error] }, result[:status]) end - - error!({ error: 'Extension weeks can not be 0.' }, 403) if params[:weeks_requested] == 0 - - max_duration = task.weeks_can_extend - duration = params[:weeks_requested] - duration = max_duration unless params[:weeks_requested] <= max_duration - - error!({ error: 'Extensions cannot be granted beyond task deadline.' }, 403) if duration <= 0 - - result = task.apply_for_extension(current_user, params[:comment], duration) - present result.serialize(current_user), Grape::Presenters::Presenter end desc 'Assess an extension for a task' diff --git a/app/services/extension_service.rb b/app/services/extension_service.rb index 59cc7c4a2..323b430b5 100644 --- a/app/services/extension_service.rb +++ b/app/services/extension_service.rb @@ -19,8 +19,8 @@ def self.grant_extension(project_id, task_definition_id, user, weeks_requested, # ===== Student Request Logic (current endpoint) ===== unless is_staff_grant - # Check task-level authorization for student requests - unless AuthorisationHelpers.authorise?(user, task, :request_extension) + # 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 From 66d8bf71df8ee65459b2e6ab66f06cf9ee6bdc8e Mon Sep 17 00:00:00 2001 From: Sahiru Withanage <165999067+SahiruWithanage@users.noreply.github.com> Date: Sun, 11 May 2025 14:46:07 +1000 Subject: [PATCH 4/4] Change suggested comments changes. --- app/services/extension_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/extension_service.rb b/app/services/extension_service.rb index 323b430b5..01af8cc4c 100644 --- a/app/services/extension_service.rb +++ b/app/services/extension_service.rb @@ -17,7 +17,7 @@ def self.grant_extension(project_id, task_definition_id, user, weeks_requested, # 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) ===== + # ===== 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) })