From 7b5a001ba70e52cf423f8cfd8400a5f8d161c382 Mon Sep 17 00:00:00 2001 From: Sahiru Withanage Date: Sun, 6 Apr 2025 18:54:24 +1000 Subject: [PATCH 1/9] 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/9] 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/9] 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 c454932bdcfd117b17de7441f4640630f2b97e5d Mon Sep 17 00:00:00 2001 From: SahiruWithanage Date: Sun, 27 Apr 2025 15:51:11 +1000 Subject: [PATCH 4/9] feat(notifications): send notifications when extensions are granted Implemented backend logic to send emails to tutor and student when extensions are granted. Also enable it so the front end can use the returned information from the api to display notifications. --- app/api/staff_grant_extension_api.rb | 25 +++ app/mailers/notifications_mailer.rb | 79 +++++++- .../extension_granted.html.erb | 124 +++++++++++++ .../extension_granted.text.erb | 38 ++++ config/environments/test.rb | 3 + test/mailers/notifications_mailer_test.rb | 168 ++++++++++++++++++ 6 files changed, 435 insertions(+), 2 deletions(-) create mode 100644 app/views/notifications_mailer/extension_granted.html.erb create mode 100644 app/views/notifications_mailer/extension_granted.text.erb create mode 100644 test/mailers/notifications_mailer_test.rb diff --git a/app/api/staff_grant_extension_api.rb b/app/api/staff_grant_extension_api.rb index 9c50aa48b..e3d113b13 100644 --- a/app/api/staff_grant_extension_api.rb +++ b/app/api/staff_grant_extension_api.rb @@ -115,9 +115,34 @@ class StaffGrantExtensionApi < Grape::API error!({ error: 'Some extensions failed to be granted', results: results }, 400) end + # Send notifications only if successful and after processing all students + if results[:successful].any? + successful_extensions = results[:successful].map do |result| + # Re-fetch project within the transaction to ensure consistency + project = Project.find(result[:project_id]) + task = project.task_for_task_definition(task_definition) + # Ensure we get the latest extension comment created within this transaction + task.all_comments.where(content_type: :extension).order(created_at: :desc).first + end + + # Filter out any nil results in case a comment wasn't found (shouldn't happen ideally) + successful_extensions.compact! + + if successful_extensions.any? + NotificationsMailer.extension_granted( + successful_extensions, + current_user, + params[:student_ids].count, + results[:failed], + true # is_staff_grant = true + ).deliver_later + end + end + status 201 present results, with: Grape::Presenters::Presenter end + rescue ActiveRecord::RecordNotFound error!({ error: 'Unit or task definition not found' }, 404) rescue StandardError diff --git a/app/mailers/notifications_mailer.rb b/app/mailers/notifications_mailer.rb index 741fa18b8..2fd1c11aa 100644 --- a/app/mailers/notifications_mailer.rb +++ b/app/mailers/notifications_mailer.rb @@ -1,7 +1,20 @@ class NotificationsMailer < ActionMailer::Base + + # Load configuration values at class level + def self.doubtfire_host + Doubtfire::Application.config.institution[:host] || 'doubtfire.deakin.edu.au' + end + + def self.doubtfire_product_name + Doubtfire::Application.config.institution[:product_name] || 'Doubtfire' + end + + # Set default from address using class methods + default from: -> { "#{self.class.doubtfire_product_name} <#{@granted_by&.email}>" } + def add_general - @doubtfire_host = Doubtfire::Application.config.institution[:host] - @doubtfire_product_name = Doubtfire::Application.config.institution[:product_name] + @doubtfire_host = self.class.doubtfire_host + @doubtfire_product_name = self.class.doubtfire_product_name @unsubscribe_url = "#{@doubtfire_host}/#/home?notifications" end @@ -88,6 +101,68 @@ def this_these(num) end end + # Sends a summary email to the staff member who granted the extensions + def extension_granted_summary(extensions, granted_by, total_selected, failed_extensions = []) + @granted_by = granted_by + @extensions = extensions + @total_selected = total_selected + @failed_extensions = failed_extensions + @unit = extensions.any? ? extensions.first.task.unit : nil + @is_tutor = true + + add_general + + email_with_name = %("#{@granted_by.name}" <#{@granted_by.email}>) + mail( + to: email_with_name, + subject: @unit ? "#{@unit.name}: Staff Grant Extensions" : "Staff Grant Extensions", + template_name: 'extension_granted' + ) + end + + # Sends a notification to a student about their granted extension + def extension_granted_notification(extension, granted_by) + @granted_by = granted_by + @extension = extension + @task = extension.task + @student = extension.project.student + @is_tutor = false + + add_general + + email_with_name = %("#{@student.name}" <#{@student.email}>) + tutor_email = %("#{@granted_by.name}" <#{@granted_by.email}>) + + mail( + to: email_with_name, + from: tutor_email, + subject: "#{@task.unit.name}: Extension granted for #{@task.task_definition.name}", + template_name: 'extension_granted' + ) + end + + # Main method to handle extension notifications from staff + def extension_granted(extensions, granted_by, total_selected, failed_extensions = [], is_staff_grant = false) + # Only send notifications for staff-granted bulk extensions + return unless is_staff_grant && (extensions.any? || failed_extensions.any?) + + begin + # Send summary to staff member who granted the extensions + extension_granted_summary(extensions, granted_by, total_selected, failed_extensions).deliver_now + + # Send individual notifications only to students who have enabled email notifications + extensions.each do |extension| + student = extension.project.student + if student.receive_task_notifications + extension_granted_notification(extension, granted_by).deliver_now + end + end + rescue => e + Rails.logger.error "Failed to send extension notifications: #{e.message}" + Rails.logger.error e.backtrace.join("\n") + end + end + helper_method :top_task_desc helper_method :were_was helper_method :are_is diff --git a/app/views/notifications_mailer/extension_granted.html.erb b/app/views/notifications_mailer/extension_granted.html.erb new file mode 100644 index 000000000..d3eb9a397 --- /dev/null +++ b/app/views/notifications_mailer/extension_granted.html.erb @@ -0,0 +1,124 @@ + + + + + + + +
+

Extension Granted

+
+ +
+ <% if @is_tutor %> +

You have granted extensions for the following students:

+ + + + + + + + + + + <% @extensions.each do |extension| %> + + + + + + <% end %> + +
StudentTaskNew Due Date
<%= extension.project.student.name %><%= extension.task.task_definition.name %><%= extension.task.due_date.strftime("%d %b %Y") %>
+ + <% if @failed_extensions.any? %> +

Failed Extensions

+ + + + + + + + + <% @failed_extensions.each do |failed| %> + + + + + <% end %> + +
Student IDError
<%= failed[:student_id] %><%= failed[:error] %>
+ <% end %> + +

Total students selected: <%= @total_selected %>

+

Successfully granted: <%= @extensions.count %>

+ <% if @failed_extensions.any? %> +

Failed: <%= @failed_extensions.count %>

+ <% end %> + <% else %> +

Dear <%= @student.name %>,

+ +

An extension has been granted for your task: <%= @task.task_definition.name %>

+ +

Details:

+
    +
  • New due date: <%= @task.due_date.strftime("%d %b %Y") %>
    • +
  • Granted by: <%= @granted_by.name %>
    • + <% if @extension.comment.present? %> +
  • Comment: <%= @extension.comment %>
    • + <% end %> +
+ <% end %> +
+ +
+

This is an automated message from Doubtfire.

+
+ + diff --git a/app/views/notifications_mailer/extension_granted.text.erb b/app/views/notifications_mailer/extension_granted.text.erb new file mode 100644 index 000000000..f1d986465 --- /dev/null +++ b/app/views/notifications_mailer/extension_granted.text.erb @@ -0,0 +1,38 @@ +<% if @is_tutor %> +You have granted extensions for the following students: + +Extensions granted: +<% @extensions.each do |extension| %> +- <%= extension.project.student.name %>: <%= extension.task.task_definition.name %> + New due date: <%= extension.task.due_date.strftime("%B %d, %Y") %> +<% end %> + +Summary: +- Total selected for extension: <%= @total_selected %> +- Successfully granted: <%= @extensions.count %> +<% if @failed_extensions.present? %> +- Failed to grant: <%= @failed_extensions.count %> + +Failed extensions: +<% @failed_extensions.each do |failed| %> +- Student ID <%= failed[:student_id] %>: <%= failed[:error] %> +<% end %> +<% end %> +<% else %> +Dear <%= @student.name %>, + +An extension has been granted for your task: <%= @task.task_definition.name %> + +Details: +- New due date: <%= @task.due_date.strftime("%B %d, %Y") %> +- Granted by: <%= @granted_by.name %> +<% if @extension.comment.present? %> +- Comment: <%= @extension.comment %> +<% end %> +<% end %> + +Cheers, +The <%= @doubtfire_product_name %> Team + +--- +To unsubscribe from these notifications, visit: <%= @unsubscribe_url %> diff --git a/config/environments/test.rb b/config/environments/test.rb index a497efc06..80ebf1677 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -27,6 +27,9 @@ # The :test delivery method accumulates sent emails in the # ActionMailer::Base.deliveries array. config.action_mailer.delivery_method = :test + config.action_mailer.perform_deliveries = true + config.action_mailer.raise_delivery_errors = true + config.action_mailer.default_url_options = { host: 'test.host' } # Print deprecation notices to the stderr config.active_support.deprecation = :stderr diff --git a/test/mailers/notifications_mailer_test.rb b/test/mailers/notifications_mailer_test.rb new file mode 100644 index 000000000..d353839fb --- /dev/null +++ b/test/mailers/notifications_mailer_test.rb @@ -0,0 +1,168 @@ +require 'test_helper' + +class NotificationsMailerTest < ActionMailer::TestCase + include TestHelpers::AuthHelper + + def setup + # Mock Doubtfire configuration + Doubtfire::Application.config.institution = { + host: 'doubtfire.deakin.edu.au', + product_name: 'Doubtfire' + } + + # Create unit and staff + @unit = FactoryBot.create(:unit) + @staff = FactoryBot.create(:user, role: Role.tutor) + @unit.employ_staff(@staff, Role.tutor) + + # Create a task definition + @task_definition = @unit.task_definitions.create!({ + tutorial_stream: @unit.tutorial_streams.first, + name: 'Test Task', + description: 'Test task for notifications', + 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: 'TESTTASK', + restrict_status_updates: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + + # Create students and projects with notification preferences + @students = [] + @projects = [] + + # Create one student with notifications enabled + student_with_notifications = FactoryBot.create(:user, role: Role.student) + student_with_notifications.update(receive_task_notifications: true) + project = @unit.projects.create!(user: student_with_notifications, enrolled: true) + @students << student_with_notifications + @projects << project + + # Create two students without notifications + 2.times do + student = FactoryBot.create(:user, role: Role.student) + student.update(receive_task_notifications: false) + project = @unit.projects.create!(user: student, enrolled: true) + @students << student + @projects << project + end + + # Clear any existing emails before each test + ActionMailer::Base.deliveries.clear + end + + def teardown + @task_definition.destroy! + @unit.destroy! + ActionMailer::Base.deliveries.clear + end + + test 'creates correct extension summary email' do + # Create extensions + extensions = [] + @projects.each do |project| + task = project.task_for_task_definition(@task_definition) + extension = task.apply_for_extension(@staff, 'Test comment', 1) + extension.assess_extension(@staff, true, true) + extensions << extension + end + + # Get the mail object + mail = NotificationsMailer.extension_granted_summary(extensions, @staff, extensions.count) + + # Verify email properties + assert_equal [@staff.email], mail.to + assert_equal "#{@unit.name}: Staff Grant Extensions", mail.subject + assert_match /You have granted extensions for the following students/, mail.html_part.body.to_s + end + + test 'creates correct extension notification email' do + # Create extension + project = @projects.first + task = project.task_for_task_definition(@task_definition) + extension = task.apply_for_extension(@staff, 'Test comment', 1) + extension.assess_extension(@staff, true, true) + + # Get the mail object + mail = NotificationsMailer.extension_granted_notification(extension, @staff) + + # Verify email properties + assert_equal [@students.first.email], mail.to + assert_equal "#{@unit.name}: Extension granted for #{@task_definition.name}", mail.subject + assert_match /Dear #{@students.first.name}/, mail.html_part.body.to_s + end + + test 'creates correct extension summary with failed extensions' do + # Create successful extensions + successful_extensions = [] + @projects.each do |project| + task = project.task_for_task_definition(@task_definition) + extension = task.apply_for_extension(@staff, 'Test comment', 1) + extension.assess_extension(@staff, true, true) + successful_extensions << extension + end + + # Create failed extensions + failed_extensions = [ + { student_id: 999, error: 'Student not found in unit' }, + { student_id: 1000, error: 'Extension cannot be granted beyond task deadline' } + ] + + # Get the mail object + mail = NotificationsMailer.extension_granted_summary( + successful_extensions, + @staff, + successful_extensions.count, + failed_extensions + ) + + # Verify email includes failed extensions + assert_equal [@staff.email], mail.to + assert_match /Failed Extensions/, mail.html_part.body.to_s + assert_match /999/, mail.html_part.body.to_s + assert_match /1000/, mail.html_part.body.to_s + end + + test 'creates correct extension notification with special characters' do + # Create task with special characters + special_task = @unit.task_definitions.create!({ + tutorial_stream: @unit.tutorial_streams.first, + name: 'Test Task with !@#$%^&*()', + description: 'Test task with special characters', + 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: 'SPECIAL', + restrict_status_updates: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + + # Create extension + project = @projects.first + task = project.task_for_task_definition(special_task) + extension = task.apply_for_extension(@staff, 'Special characters test', 1) + extension.assess_extension(@staff, true, true) + + # Get the mail object + mail = NotificationsMailer.extension_granted_notification(extension, @staff) + + # Verify email handles special characters + assert_equal [@students.first.email], mail.to + assert_equal "#{@unit.name}: Extension granted for #{special_task.name}", mail.subject + assert_match /Dear #{@students.first.name}/, mail.html_part.body.to_s + + # Clean up + special_task.destroy! + end +end From 35b2eabc3aed10a54d86a51710e7c1f6dc7caf7f Mon Sep 17 00:00:00 2001 From: Sahiru Withanage <165999067+SahiruWithanage@users.noreply.github.com> Date: Sun, 11 May 2025 22:40:43 +1000 Subject: [PATCH 5/9] Make a comment line change A comment line change made in the staff grant extension feature branch that hasn't been updated here. Changing to keep the consistency. --- 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) }) From 298460d0378e53ef81d7d17f36bd889501fe2793 Mon Sep 17 00:00:00 2001 From: samindiii Date: Sun, 18 May 2025 16:56:39 +1000 Subject: [PATCH 6/9] Add notification table, model and define relation with user --- app/models/notification.rb | 3 + app/models/user.rb | 1 + .../20250518011250_create_notifications.rb | 10 +++ db/schema.rb | 77 ++++++++++--------- 4 files changed, 56 insertions(+), 35 deletions(-) create mode 100644 app/models/notification.rb create mode 100644 db/migrate/20250518011250_create_notifications.rb diff --git a/app/models/notification.rb b/app/models/notification.rb new file mode 100644 index 000000000..c99183b19 --- /dev/null +++ b/app/models/notification.rb @@ -0,0 +1,3 @@ +class Notification < ApplicationRecord + belongs_to :user +end diff --git a/app/models/user.rb b/app/models/user.rb index 6e016badf..ebc43f003 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -136,6 +136,7 @@ def token_for_text?(a_token) has_many :projects, dependent: :destroy has_many :auth_tokens, dependent: :destroy has_one :webcal, dependent: :destroy + has_many :notifications, dependent: :destroy # Model validations/constraints validates :first_name, presence: true diff --git a/db/migrate/20250518011250_create_notifications.rb b/db/migrate/20250518011250_create_notifications.rb new file mode 100644 index 000000000..0f8326726 --- /dev/null +++ b/db/migrate/20250518011250_create_notifications.rb @@ -0,0 +1,10 @@ +class CreateNotifications < ActiveRecord::Migration[7.1] + def change + create_table :notifications do |t| + t.integer :user_id + t.string :message + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 6daa71ebf..be2da1ae5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,8 +10,8 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_05_28_223908) do - create_table "activity_types", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| +ActiveRecord::Schema[7.1].define(version: 2025_05_18_011250) do + create_table "activity_types", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false t.datetime "created_at", null: false @@ -20,21 +20,21 @@ t.index ["name"], name: "index_activity_types_on_name", unique: true end - create_table "auth_tokens", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "auth_tokens", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "auth_token_expiry", null: false t.bigint "user_id" t.string "authentication_token", null: false t.index ["user_id"], name: "index_auth_tokens_on_user_id" end - create_table "breaks", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "breaks", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "start_date", null: false t.integer "number_of_weeks", null: false t.bigint "teaching_period_id" t.index ["teaching_period_id"], name: "index_breaks_on_teaching_period_id" end - create_table "campuses", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "campuses", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name", null: false t.integer "mode", null: false t.string "abbreviation", null: false @@ -44,7 +44,7 @@ t.index ["name"], name: "index_campuses_on_name", unique: true end - create_table "comments_read_receipts", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "comments_read_receipts", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_comment_id", null: false t.bigint "user_id", null: false t.datetime "created_at", null: false @@ -54,7 +54,7 @@ t.index ["user_id"], name: "index_comments_read_receipts_on_user_id" end - create_table "discussion_comments", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "discussion_comments", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "time_started" t.datetime "time_completed" t.integer "number_of_prompts" @@ -62,7 +62,7 @@ t.datetime "updated_at", null: false end - create_table "group_memberships", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "group_memberships", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "group_id" t.bigint "project_id" t.boolean "active", default: true @@ -72,7 +72,7 @@ t.index ["project_id"], name: "index_group_memberships_on_project_id" end - create_table "group_sets", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "group_sets", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "unit_id" t.string "name" t.boolean "allow_students_to_create_groups", default: true @@ -85,7 +85,7 @@ t.index ["unit_id"], name: "index_group_sets_on_unit_id" end - create_table "group_submissions", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "group_submissions", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "group_id" t.string "notes" t.bigint "submitted_by_project_id" @@ -97,7 +97,7 @@ t.index ["task_definition_id"], name: "index_group_submissions_on_task_definition_id" end - create_table "groups", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "groups", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "group_set_id" t.bigint "tutorial_id" t.string "name" @@ -109,7 +109,7 @@ t.index ["tutorial_id"], name: "index_groups_on_tutorial_id" end - create_table "learning_outcome_task_links", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "learning_outcome_task_links", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.text "description" t.integer "rating" t.bigint "task_definition_id" @@ -122,7 +122,7 @@ t.index ["task_id"], name: "index_learning_outcome_task_links_on_task_id" end - create_table "learning_outcomes", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "learning_outcomes", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "unit_id" t.integer "ilo_number" t.string "name" @@ -131,7 +131,7 @@ t.index ["unit_id"], name: "index_learning_outcomes_on_unit_id" end - create_table "logins", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "logins", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "timestamp" t.bigint "user_id" t.datetime "created_at", null: false @@ -139,7 +139,14 @@ t.index ["user_id"], name: "index_logins_on_user_id" end - create_table "overseer_assessments", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "notifications", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.integer "user_id" + t.string "message" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + create_table "overseer_assessments", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_id", null: false t.string "submission_timestamp", null: false t.string "result_task_status" @@ -150,7 +157,7 @@ t.index ["task_id"], name: "index_overseer_assessments_on_task_id" end - create_table "overseer_images", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "overseer_images", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name", null: false t.string "tag", null: false t.datetime "created_at", null: false @@ -160,7 +167,7 @@ t.datetime "last_pulled_date" end - create_table "projects", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "projects", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "unit_id" t.string "project_role" t.datetime "created_at", null: false @@ -187,14 +194,14 @@ t.index ["user_id"], name: "index_projects_on_user_id" end - create_table "roles", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "roles", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name" t.text "description" t.datetime "created_at", null: false t.datetime "updated_at", null: false end - create_table "task_comments", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_comments", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_id", null: false t.bigint "user_id", null: false t.string "comment", limit: 4096 @@ -225,7 +232,7 @@ t.index ["user_id"], name: "index_task_comments_on_user_id" end - create_table "task_definitions", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_definitions", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "unit_id" t.string "name" t.string "description", limit: 4096 @@ -256,7 +263,7 @@ t.index ["unit_id"], name: "index_task_definitions_on_unit_id" end - create_table "task_engagements", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_engagements", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "engagement_time" t.string "engagement" t.bigint "task_id" @@ -265,7 +272,7 @@ t.index ["task_id"], name: "index_task_engagements_on_task_id" end - create_table "task_pins", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_pins", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_id", null: false t.bigint "user_id", null: false t.datetime "created_at", null: false @@ -275,7 +282,7 @@ t.index ["user_id"], name: "fk_rails_915df186ed" end - create_table "task_similarities", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_similarities", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_id" t.bigint "other_task_id" t.integer "pct" @@ -290,14 +297,14 @@ t.index ["tii_submission_id"], name: "index_task_similarities_on_tii_submission_id" end - create_table "task_statuses", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_statuses", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name" t.string "description" t.datetime "created_at", null: false t.datetime "updated_at", null: false end - create_table "task_submissions", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "task_submissions", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "submission_time" t.datetime "assessment_time" t.string "outcome" @@ -309,7 +316,7 @@ t.index ["task_id"], name: "index_task_submissions_on_task_id" end - create_table "tasks", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "tasks", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "task_definition_id" t.bigint "project_id" t.bigint "task_status_id" @@ -335,7 +342,7 @@ t.index ["task_status_id"], name: "index_tasks_on_task_status_id" end - create_table "teaching_periods", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "teaching_periods", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "period", null: false t.datetime "start_date", null: false t.datetime "end_date", null: false @@ -394,7 +401,7 @@ t.index ["tii_task_similarity_id"], name: "index_tii_submissions_on_tii_task_similarity_id" end - create_table "tutorial_enrolments", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "tutorial_enrolments", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false t.bigint "project_id", null: false @@ -404,7 +411,7 @@ t.index ["tutorial_id"], name: "index_tutorial_enrolments_on_tutorial_id" end - create_table "tutorial_streams", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "tutorial_streams", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false t.datetime "created_at", null: false @@ -418,7 +425,7 @@ t.index ["unit_id"], name: "index_tutorial_streams_on_unit_id" end - create_table "tutorials", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "tutorials", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "unit_id" t.string "meeting_day" t.string "meeting_time" @@ -437,7 +444,7 @@ t.index ["unit_role_id"], name: "index_tutorials_on_unit_role_id" end - create_table "unit_roles", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "unit_roles", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "user_id" t.bigint "tutorial_id" t.datetime "created_at", null: false @@ -450,7 +457,7 @@ t.index ["user_id"], name: "index_unit_roles_on_user_id" end - create_table "units", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "units", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "name" t.string "description", limit: 4096 t.datetime "start_date" @@ -480,7 +487,7 @@ t.index ["teaching_period_id"], name: "index_units_on_teaching_period_id" end - create_table "users", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "users", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false t.string "reset_password_token" @@ -513,7 +520,7 @@ t.index ["role_id"], name: "index_users_on_role_id" end - create_table "webcal_unit_exclusions", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "webcal_unit_exclusions", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.bigint "webcal_id", null: false t.bigint "unit_id", null: false t.index ["unit_id", "webcal_id"], name: "index_webcal_unit_exclusions_on_unit_id_and_webcal_id", unique: true @@ -521,7 +528,7 @@ t.index ["webcal_id"], name: "fk_rails_d5fab02cb7" end - create_table "webcals", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| + create_table "webcals", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| t.string "guid", limit: 36, null: false t.boolean "include_start_dates", default: false, null: false t.bigint "user_id" From a984ce357e2b7e0e4bf7fc5767464f918aa802f4 Mon Sep 17 00:00:00 2001 From: samindiii Date: Sun, 18 May 2025 16:57:45 +1000 Subject: [PATCH 7/9] Define notification api's to GET and DELETE notification by id --- app/api/api_root.rb | 1 + app/api/notifications_api.rb | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 app/api/notifications_api.rb diff --git a/app/api/api_root.rb b/app/api/api_root.rb index 09e9f0787..c13975473 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -58,6 +58,7 @@ class ApiRoot < Grape::API mount GroupSetsApi mount LearningOutcomesApi mount LearningAlignmentApi + mount NotificationsApi mount ProjectsApi mount SettingsApi mount StaffGrantExtensionApi diff --git a/app/api/notifications_api.rb b/app/api/notifications_api.rb new file mode 100644 index 000000000..371b08c5b --- /dev/null +++ b/app/api/notifications_api.rb @@ -0,0 +1,29 @@ +class NotificationsApi < Grape::API + helpers AuthenticationHelpers + helpers AuthorisationHelpers + + before do + authenticated? + end + + desc 'Get current user notifications' + get '/notifications' do + notifications = current_user.notifications.order(created_at: :desc) + # Return array of notifications as JSON (id and message only) + notifications.as_json(only: [:id, :message]) + end + + desc 'Delete user notification by id' + delete '/notifications/:id' do + notification = current_user.notifications.find_by(id: params[:id]) + error!({ error: 'Notification not found' }, 404) unless notification + notification.destroy + status 204 + end + + desc 'Delete all user notifications' + delete '/notifications' do + current_user.notifications.delete_all + status 204 + end +end From bef3055365363646937d11202b95ab23d189b0ad Mon Sep 17 00:00:00 2001 From: samindiii Date: Sun, 18 May 2025 16:58:59 +1000 Subject: [PATCH 8/9] Create in-system notifications for students with successfull extensions --- app/api/staff_grant_extension_api.rb | 11 +++++++++++ test/api/staff_grant_extension_test.rb | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/app/api/staff_grant_extension_api.rb b/app/api/staff_grant_extension_api.rb index e3d113b13..254f8b5fb 100644 --- a/app/api/staff_grant_extension_api.rb +++ b/app/api/staff_grant_extension_api.rb @@ -136,6 +136,17 @@ class StaffGrantExtensionApi < Grape::API results[:failed], true # is_staff_grant = true ).deliver_later + + # Create in-system notifications for successful extensions + results[:successful].each do |result| + student = User.find_by(id: result[:student_id]) + next unless student + + Notification.create!( + user_id: student.id, + message: "#{unit.name}: You were granted an extension for task '#{task_definition.name}'." + ) + end end end diff --git a/test/api/staff_grant_extension_test.rb b/test/api/staff_grant_extension_test.rb index f03b9a62e..a90d74dcd 100644 --- a/test/api/staff_grant_extension_test.rb +++ b/test/api/staff_grant_extension_test.rb @@ -53,6 +53,13 @@ def test_staff_grant_extension_success assert response["successful"][0]["extension_response"].present?, 'Should have extension response' assert response["successful"][0]["task_status"].present?, 'Should have task status' + notifications = Notification.where(user_id: project.student.id) + assert_equal 1, notifications.count, 'Should create one notification for the student' + notification = notifications.first + assert_match /You were granted an extension for task/, notification.message + assert_match /#{td.name}/, notification.message + assert_match /#{unit.name}/, notification.message + td.destroy! unit.destroy! end From 7b2fb324893d42040384aef2b4f36437332b29b5 Mon Sep 17 00:00:00 2001 From: samindiii Date: Sun, 10 Aug 2025 17:48:24 +1000 Subject: [PATCH 9/9] Fix review comments - Add notifcation factory - Add unit tests for notifications - Only send first 20 notifications to front end - Have a max characters of 500 --- app/api/notifications_api.rb | 5 ++- app/models/notification.rb | 2 + test/api/notifications_api_test.rb | 54 +++++++++++++++++++++++++ test/factories/notifications_factory.rb | 6 +++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 test/api/notifications_api_test.rb create mode 100644 test/factories/notifications_factory.rb diff --git a/app/api/notifications_api.rb b/app/api/notifications_api.rb index 371b08c5b..c746d2490 100644 --- a/app/api/notifications_api.rb +++ b/app/api/notifications_api.rb @@ -8,8 +8,9 @@ class NotificationsApi < Grape::API desc 'Get current user notifications' get '/notifications' do - notifications = current_user.notifications.order(created_at: :desc) - # Return array of notifications as JSON (id and message only) + notifications = current_user.notifications + .order(created_at: :desc) + .limit(20) notifications.as_json(only: [:id, :message]) end diff --git a/app/models/notification.rb b/app/models/notification.rb index c99183b19..633241ab5 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,3 +1,5 @@ class Notification < ApplicationRecord belongs_to :user + + validates :message, presence: true, length: { maximum: 500 } end diff --git a/test/api/notifications_api_test.rb b/test/api/notifications_api_test.rb new file mode 100644 index 000000000..26ca4186f --- /dev/null +++ b/test/api/notifications_api_test.rb @@ -0,0 +1,54 @@ +require 'test_helper' + +class NotificationsApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def setup + @user = FactoryBot.create(:user) + @notifications = FactoryBot.create_list(:notification, 3, user: @user) + add_auth_header_for user: @user + end + + def test_get_notifications + get '/api/notifications' + assert_equal 200, last_response.status + body = last_response_body + assert_equal 3, body.length + assert body.first.key?('message') + end + + def test_delete_single_notification + note = @notifications.first + delete "/api/notifications/#{note.id}" + assert_equal 204, last_response.status + assert_nil Notification.find_by(id: note.id) + end + + def test_delete_all_notifications + delete '/api/notifications' + assert_equal 204, last_response.status + assert_equal 0, Notification.where(user_id: @user.id).count + end + + def test_get_notifications_limits_to_20 + # Create 25 notifications for the user + FactoryBot.create_list(:notification, 25, user: @user) + + get '/api/notifications' + assert_equal 200, last_response.status + + body = last_response_body + assert_equal 20, body.length, 'Should only return the latest 20 notifications' + + # Verify notifications are ordered newest first by created_at (or id) + first_received = body.first['id'] + last_received = body.last['id'] + assert first_received > last_received, 'Notifications should be ordered newest first' + end +end diff --git a/test/factories/notifications_factory.rb b/test/factories/notifications_factory.rb new file mode 100644 index 000000000..deefc4a03 --- /dev/null +++ b/test/factories/notifications_factory.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :notification do + association :user + sequence(:message) { |n| "Test notification message #{n}" } + end +end