From 849990fd53e40865bec16ee037924d412b99a0c1 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 27 Sep 2026 19:58:33 +1000 Subject: [PATCH 1/2] fix: T2 2026 task, comment and user fixes Brings the T2 2026 core work from ontrack-features-t2-2026 11.0.x (reviewed and merged work) onto thoth-tech 11.0.x. Co-authored-by: maplefoxgit Co-authored-by: Tan Tai Co-authored-by: Shaashwat3 Co-authored-by: JOSHUA ERICKSON Co-authored-by: jmirchh75 Co-authored-by: anaghwadhwa123 --- .ci-setup/crontab | 1 - app/api/api_root.rb | 24 + app/api/courseflow_api.rb | 117 ++++ app/api/discussion_comment_api.rb | 8 +- app/api/entities/comment_entity.rb | 7 +- .../entities/minimal/minimal_user_entity.rb | 1 + app/api/entities/project_entity.rb | 2 +- app/api/entities/user_entity.rb | 23 + app/api/overseer_steps_api.rb | 7 +- app/api/settings_api.rb | 36 +- app/api/task_comments_api.rb | 104 ++- app/api/tasks_api.rb | 72 ++- app/api/users_api.rb | 92 ++- app/controllers/readiness_controller.rb | 5 + app/models/comments/task_comment.rb | 75 ++- app/models/courseflow/course.rb | 115 ++++ app/models/courseflow/course_map.rb | 111 ++++ app/models/unit.rb | 38 +- app/services/courseflow/catalog_importer.rb | 42 ++ app/services/readiness_check.rb | 28 + app/sidekiq/check_unit_similarity_job.rb | 57 ++ ...830063140_add_theme_preference_to_users.rb | 8 + docs/display-name-migration.md | 113 ++++ test/api/auth_test.rb | 61 +- test/api/comments/comment_test.rb | 303 ++++++++- test/api/discussion_comment_api_test.rb | 98 +++ test/api/task_grade_authorisation_test.rb | 144 +++++ test/api/tasks_api_test.rb | 324 +++++++++- test/api/units/similarity_scan_test.rb | 52 ++ test/api/users_test.rb | 323 +++++++++- test/models/task_test.rb | 600 +++++++++++++----- test/models/user_test.rb | 99 ++- 32 files changed, 2832 insertions(+), 258 deletions(-) create mode 100644 app/api/courseflow_api.rb create mode 100644 app/controllers/readiness_controller.rb create mode 100644 app/models/courseflow/course.rb create mode 100644 app/models/courseflow/course_map.rb create mode 100644 app/services/courseflow/catalog_importer.rb create mode 100644 app/services/readiness_check.rb create mode 100644 app/sidekiq/check_unit_similarity_job.rb create mode 100644 db/migrate/20260830063140_add_theme_preference_to_users.rb create mode 100644 docs/display-name-migration.md create mode 100644 test/api/discussion_comment_api_test.rb create mode 100644 test/api/task_grade_authorisation_test.rb create mode 100644 test/api/units/similarity_scan_test.rb diff --git a/.ci-setup/crontab b/.ci-setup/crontab index b1298d5e39..0030ad8035 100644 --- a/.ci-setup/crontab +++ b/.ci-setup/crontab @@ -4,7 +4,6 @@ PATH=/tmp/texlive/bin/x86_64-linux:/tmp/texlive/bin/aarch64-linux:/usr/local/bun 10,15,20,25,30,35,40,45,50,55 * * * * /doubtfire/lib/shell/generate_pdfs.sh 0,10,20,30,40,50 * * * * /doubtfire/lib/shell/send_overseer_notifications.sh -0 5 * * * /doubtfire/lib/shell/check_plagiarism.sh 0 8 * * * /doubtfire/lib/shell/portfolio_autogen_check.sh 0 7 * * 1 /doubtfire/lib/shell/send_weekly_emails.sh 0 1 * * * /doubtfire/lib/shell/sync_enrolments.sh diff --git a/app/api/api_root.rb b/app/api/api_root.rb index 3dbc682297..f940db579f 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -57,7 +57,10 @@ class ApiRoot < Grape::API mount Admin::OverseerAdminApi mount ActivityTypesAuthenticatedApi mount ActivityTypesPublicApi + mount CourseflowApi + mount TaskStatusesApi mount AuthenticationApi + mount AdditionalNotificationEmailVerificationApi mount BreaksApi mount DiscussionCommentApi mount EngagementsApi @@ -66,6 +69,8 @@ class ApiRoot < Grape::API mount GroupSetsApi mount LearningOutcomesApi mount ProjectsApi + mount SettingsPublicApi + mount PeerProgressApi mount SettingsApi mount StudentsApi mount Submission::PortfolioApi @@ -103,20 +108,29 @@ class ApiRoot < Grape::API mount D2lIntegrationApi::OauthPublicApi mount UsersApi + mount AdditionalNotificationEmailsApi mount WebcalApi mount WebcalPublicApi mount MarkingSessionsApi mount DiscussionPromptsApi mount OverseerStepsApi + mount TaskPrioritizationApi + mount DemoScenarioApi mount Feedback::FeedbackChipApi + # Notifications feature + mount UnitHubApi + mount NotificationsApi + mount PushSubscriptionsApi + # # Add auth details to all end points # AuthenticationHelpers.add_auth_to Admin::OverseerAdminApi AuthenticationHelpers.add_auth_to ActivityTypesAuthenticatedApi + AuthenticationHelpers.add_auth_to CourseflowApi AuthenticationHelpers.add_auth_to BreaksApi AuthenticationHelpers.add_auth_to DiscussionCommentApi AuthenticationHelpers.add_auth_to EngagementsApi @@ -125,6 +139,8 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to GroupSetsApi AuthenticationHelpers.add_auth_to LearningOutcomesApi AuthenticationHelpers.add_auth_to ProjectsApi + AuthenticationHelpers.add_auth_to SettingsApi + AuthenticationHelpers.add_auth_to PeerProgressApi AuthenticationHelpers.add_auth_to StudentsApi AuthenticationHelpers.add_auth_to Submission::PortfolioApi AuthenticationHelpers.add_auth_to Submission::PortfolioEvidenceApi @@ -149,6 +165,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to TutorialStreamsApi AuthenticationHelpers.add_auth_to TutorialEnrolmentsApi AuthenticationHelpers.add_auth_to UsersApi + AuthenticationHelpers.add_auth_to AdditionalNotificationEmailsApi AuthenticationHelpers.add_auth_to UnitRolesApi AuthenticationHelpers.add_auth_to UnitsApi AuthenticationHelpers.add_auth_to WebcalApi @@ -160,8 +177,15 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to MarkingSessionsApi AuthenticationHelpers.add_auth_to DiscussionPromptsApi AuthenticationHelpers.add_auth_to OverseerStepsApi + AuthenticationHelpers.add_auth_to TaskPrioritizationApi + AuthenticationHelpers.add_auth_to DemoScenarioApi AuthenticationHelpers.add_auth_to TutorNotesApi + # Notifications feature + AuthenticationHelpers.add_auth_to UnitHubApi + AuthenticationHelpers.add_auth_to NotificationsApi + AuthenticationHelpers.add_auth_to PushSubscriptionsApi + add_swagger_documentation \ base_path: nil, doc_version: 'v11.0.0', diff --git a/app/api/courseflow_api.rb b/app/api/courseflow_api.rb new file mode 100644 index 0000000000..49d60771ef --- /dev/null +++ b/app/api/courseflow_api.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'grape' + +# Catalog reads and private student plans. No teaching-unit permissions or IDs. +class CourseflowApi < Grape::API + helpers AuthenticationHelpers + + rescue_from Grape::Exceptions::InvalidMessageBody do + Rack::Response.new({ error: 'Plan must contain valid JSON' }.to_json, 422, + { 'content-type' => 'application/json', 'cache-control' => 'private, no-store' }) + end + + before do + authenticated? + header 'Cache-Control', 'private, no-store' + end + + helpers do + def owned_courseflow_map + Courseflow::CourseMap.where(user_id: current_user.id).find(courseflow_id(params[:id])) + end + + def courseflow_id(value) + error!({ error: 'Not found' }, 404) unless /\A[1-9]\d{0,18}\z/.match?(value.to_s) + value.to_i + end + + def courseflow_document(update: false) + request.body.rewind + raw = request.body.read(Courseflow::CatalogImporter::MAX_BYTES + 1).to_s + error!({ error: 'Plan exceeds 1 MiB' }, 422) if raw.bytesize > Courseflow::CatalogImporter::MAX_BYTES + document = JSON.parse(raw) + keys = %w[course_id name periods slots] + keys << 'lock_version' if update + unless document.is_a?(Hash) && document.keys.sort == keys.sort && !params.key?(:user_id) + error!({ error: "Plan must contain exactly #{keys.join(', ')}; ownership is server assigned" }, 422) + end + unless document['course_id'].is_a?(Integer) && document['course_id'].positive? && + document['name'].is_a?(String) && document['name'].strip.present? && document['name'].length <= 200 + error!({ error: 'course_id must be a positive integer and name a nonblank string up to 200 characters' }, 422) + end + if update && !(document['lock_version'].is_a?(Integer) && document['lock_version'].between?(0, 2_147_483_647)) + error!({ error: 'lock_version must be a nonnegative integer' }, 422) + end + document + rescue JSON::ParserError + error!({ error: 'Plan must be a JSON object' }, 422) + end + + def save_courseflow_map!(map) + map.save! + rescue ActiveRecord::RecordInvalid => e + error!({ error: 'Invalid plan', details: e.record.errors.full_messages }, 422) + rescue ActiveRecord::StaleObjectError + courseflow_conflict! + end + + def courseflow_conflict! + error!({ error: 'This plan changed in another session. Reload it before saving or deleting.' }, 409) + end + end + + namespace :courseflow do + get :courses do + Courseflow::Course.order(:code, :version, :id).map(&:as_catalog) + end + + get 'courses/:id' do + Courseflow::Course.find(courseflow_id(params[:id])).as_catalog + end + + get :maps do + Courseflow::CourseMap.where(user_id: current_user.id).includes(:course).order(updated_at: :desc, id: :desc).map(&:as_plan) + end + + get 'maps/:id' do + owned_courseflow_map.as_plan + end + + post :maps do + document = courseflow_document + course = Courseflow::Course.find_by(id: document['course_id']) + error!({ error: 'Selected course does not exist' }, 422) unless course + map = Courseflow::CourseMap.new(document.merge('user_id' => current_user.id)) + map.course = course + save_courseflow_map!(map) + status 201 + map.as_plan + end + + put 'maps/:id' do + map = owned_courseflow_map + document = courseflow_document(update: true) + map.with_lock do + courseflow_conflict! unless document['lock_version'] == map.lock_version + map.assign_attributes(document.except('lock_version')) + save_courseflow_map!(map) + end + map.as_plan + end + + delete 'maps/:id' do + map = owned_courseflow_map + unless params[:lock_version].is_a?(String) && /\A(?:0|[1-9]\d{0,9})\z/.match?(params[:lock_version]) && + params[:lock_version].to_i <= 2_147_483_647 && !params.key?(:user_id) + error!({ error: 'lock_version query parameter must be a nonnegative integer; ownership is server assigned' }, 422) + end + map.with_lock do + courseflow_conflict! unless params[:lock_version].to_i == map.lock_version + map.destroy! + end + status 204 + body false + end + end +end diff --git a/app/api/discussion_comment_api.rb b/app/api/discussion_comment_api.rb index ffd70117fc..341a1faaf0 100644 --- a/app/api/discussion_comment_api.rb +++ b/app/api/discussion_comment_api.rb @@ -30,8 +30,8 @@ class DiscussionCommentApi < Grape::API for attached_file in attached_files do if attached_file.present? - error!(error: 'Attachment is empty.') if File.size?(attached_file["tempfile"].path).blank? - error!(error: 'Attachment exceeds the maximum attachment size of 30MB.') unless File.size?(attached_file["tempfile"].path) < 30_000_000 + error!({ error: 'Attachment is empty.' }, 400) if File.size?(attached_file["tempfile"].path).blank? + error!({ error: 'Attachment exceeds the maximum attachment size of 30MB.' }, 413) unless File.size?(attached_file["tempfile"].path) < 30_000_000 end end @@ -136,8 +136,8 @@ class DiscussionCommentApi < Grape::API attached_file = params[:attachment] if attached_file.present? - error!(error: 'Attachment is empty.') if File.size?(attached_file["tempfile"].path).blank? - error!(error: 'Attachment exceeds the maximum attachment size of 30MB.') unless File.size?(attached_file["tempfile"].path) < 30_000_000 + error!({ error: 'Attachment is empty.' }, 400) if File.size?(attached_file["tempfile"].path).blank? + error!({ error: 'Attachment exceeds the maximum attachment size of 30MB.' }, 413) unless File.size?(attached_file["tempfile"].path) < 30_000_000 end logger.info("#{current_user.username} - added a reply to the discussion comment #{params[:task_comment_id]} for task #{task.id} (#{task_definition.abbreviation})") diff --git a/app/api/entities/comment_entity.rb b/app/api/entities/comment_entity.rb index 6061339cb1..3a8e506d07 100644 --- a/app/api/entities/comment_entity.rb +++ b/app/api/entities/comment_entity.rb @@ -3,7 +3,7 @@ class CommentEntity < Grape::Entity expose :id expose :comment expose :has_attachment do |data, options| - ["audio", "image", "pdf"].include?(data.content_type) + data.attachment? end expose :type do |data, options| data.content_type || "text" @@ -16,6 +16,11 @@ class CommentEntity < Grape::Entity end end expose :reply_to_id + expose :attachment_file_name, if: ->(data, _) { data.attachment? } + expose :attachment_mime_type, if: ->(data, _) { data.attachment? } + expose :attachment_byte_size, if: ->(data, _) { data.attachment? } do |data, _| + data.attachment_size + end expose :created_at expose :recipient_read_time, safe: true expose :author do |data, options| diff --git a/app/api/entities/minimal/minimal_user_entity.rb b/app/api/entities/minimal/minimal_user_entity.rb index 230875bf38..6e59e711e5 100644 --- a/app/api/entities/minimal/minimal_user_entity.rb +++ b/app/api/entities/minimal/minimal_user_entity.rb @@ -7,6 +7,7 @@ class MinimalUserEntity < Grape::Entity expose :last_name expose :username expose :nickname + expose :display_name end end end diff --git a/app/api/entities/project_entity.rb b/app/api/entities/project_entity.rb index ebe150267f..ac4a26c955 100644 --- a/app/api/entities/project_entity.rb +++ b/app/api/entities/project_entity.rb @@ -21,7 +21,7 @@ class ProjectEntity < Grape::Entity expose :task_stats, as: :stats, unless: :for_student - expose :tasks, using: TaskEntity, unless: :summary_only do |project, options| + expose :tasks, using: TaskEntity, if: ->(project, options) { !options[:summary_only] || options[:include_task_definitions] } do |project, options| project.task_details_for_shallow_serializer(options[:user]) end diff --git a/app/api/entities/user_entity.rb b/app/api/entities/user_entity.rb index 1a1155e103..2a8306f2c1 100644 --- a/app/api/entities/user_entity.rb +++ b/app/api/entities/user_entity.rb @@ -7,11 +7,34 @@ class UserEntity < Grape::Entity expose :last_name expose :username expose :nickname + expose :display_name expose :receive_task_notifications, unless: :minimal expose :receive_portfolio_notifications, unless: :minimal expose :receive_feedback_notifications, unless: :minimal + expose :display_peer_progress, unless: :minimal expose :opt_in_to_research, unless: :minimal expose :has_run_first_time_setup, unless: :minimal + # Theme preference is account-private presentation state. Only endpoints + # serialising the authenticated account opt in to these fields; shared user + # lookups must not disclose either the choice or when it was made. + expose :theme_preference, + unless: :minimal, + if: lambda { |user, options| + options.key?(:theme_owner_id) && user.id.present? && options[:theme_owner_id] == user.id + } + expose :theme_preference_updated_at, + unless: :minimal, + if: lambda { |user, options| + options.key?(:theme_owner_id) && user.id.present? && options[:theme_owner_id] == user.id + } + + expose :institutional_identity_managed, unless: :minimal do |_user, _options| + !AuthenticationHelpers.db_auth? + end + + expose :email_editable, unless: :minimal do |_user, _options| + AuthenticationHelpers.db_auth? + end expose :accepted_tii_eula, unless: :minimal, if: ->(user, options) { TurnItIn.enabled? } do |user, options| if TiiActionFetchFeaturesEnabled.eula_required? diff --git a/app/api/overseer_steps_api.rb b/app/api/overseer_steps_api.rb index 018f3cc547..85d4b6661e 100644 --- a/app/api/overseer_steps_api.rb +++ b/app/api/overseer_steps_api.rb @@ -203,7 +203,12 @@ class OverseerStepsApi < Grape::API unit = project.unit - overseer_assessment = OverseerAssessment.find(params[:id]) + # Look the assessment up through the project and task definition in the url, so that + # an id from outside the authorised project raises RecordNotFound and returns a 404. + overseer_assessment = OverseerAssessment.joins(:task) + .where(tasks: { project_id: project.id, task_definition_id: params[:task_def_id] }) + .find(params[:id]) + present overseer_assessment.overseer_step_results, with: Entities::OverseerStepResultEntity, my_role: unit.role_for(current_user) end end diff --git a/app/api/settings_api.rb b/app/api/settings_api.rb index 49968392ca..e6af65ce80 100644 --- a/app/api/settings_api.rb +++ b/app/api/settings_api.rb @@ -1,29 +1,29 @@ require 'grape' class SettingsApi < Grape::API - # - # Returns the current auth method - # - desc 'Return configurable details for the Doubtfire front end' + helpers AuthenticationHelpers + + before do + authenticated? + end + + desc 'Return authenticated feature configuration for the Doubtfire front end' get '/settings' do response = { - externalName: Doubtfire::Application.config.institution[:product_name], - hasLogo: Doubtfire::Application.config.institution[:has_logo], - logoUrl: Doubtfire::Application.config.institution[:logo_url], - logoLinkUrl: Doubtfire::Application.config.institution[:logo_link_url], overseerEnabled: Doubtfire::Application.config.overseer_enabled, tiiEnabled: TurnItIn.enabled?, - d2lEnabled: D2lIntegration.enabled? - } + d2lEnabled: D2lIntegration.enabled?, + tutorialEnabled: Doubtfire::Application.config.tutorial_enabled, - present response, with: Grape::Presenters::Presenter - end - - desc 'Return privacy policy details' - get '/settings/privacy' do - response = { - privacy: Doubtfire::Application.config.institution[:privacy], - plagiarism: Doubtfire::Application.config.institution[:plagiarism] + # Web push. The VAPID *public* key is not a secret — the browser has to + # send it to the push service to subscribe at all. Serving it here means it + # is configured in one place instead of being copied into the front end and + # going stale the first time the keys are rotated. + # + # Blank when push is not configured, which is how the client knows not to + # offer the opt-in. + pushEnabled: PushNotificationService.configured?, + vapidPublicKey: ENV.fetch('DOUBTFIRE_VAPID_PUBLIC_KEY', nil).presence } present response, with: Grape::Presenters::Presenter diff --git a/app/api/task_comments_api.rb b/app/api/task_comments_api.rb index cfe2500a87..1434fb5736 100644 --- a/app/api/task_comments_api.rb +++ b/app/api/task_comments_api.rb @@ -9,11 +9,39 @@ class TaskCommentsApi < Grape::API authenticated? end + helpers do + # A retry that overlaps its original request can miss the lookup at the + # start of the create endpoint and then lose the race at the unique index, + # at the uniqueness validation, or at add_text_comment's duplicate guard. + # In each case the comment already stored for this id is the answer. + def comment_for_client_request(task, client_request_id) + return nil if client_request_id.blank? + + # Skip the request's query cache. It still holds the first lookup's + # miss, and the comment was stored since by the request this one raced. + TaskComment.uncached do + task.comments.find_by(user_id: current_user.id, client_request_id: client_request_id) + end + end + + def lost_client_request_race?(error) + error.is_a?(ActiveRecord::RecordNotUnique) || + (error.is_a?(ActiveRecord::RecordInvalid) && error.record.errors.of_kind?(:client_request_id, :taken)) + end + end + + desc 'Get the server-owned task chat attachment policy' + get '/task_comments/upload_policy' do + present CommentAttachmentPolicy.public_policy + end + desc 'Add a new comment to a task' params do optional :comment, type: String, desc: 'The comment text to add to the task' - optional :attachment, type: File, desc: 'Image, sound, PDF or video comment file' + optional :attachment, type: File, desc: 'Approved image, sound, PDF, Word or spreadsheet attachment' optional :reply_to_id, type: Integer, desc: 'The comment to which this comment is replying' + optional :client_request_id, type: String, regexp: /\A[0-9a-f-]{1,64}\z/i, + desc: 'Stable client-generated identifier used to make attachment retries idempotent' end post '/projects/:project_id/task_def_id/:task_definition_id/comments' do project = Project.find(params[:project_id]) @@ -26,6 +54,7 @@ class TaskCommentsApi < Grape::API text_comment = params[:comment] attached_file = params[:attachment] reply_to_id = params[:reply_to_id] + client_request_id = params[:client_request_id] task = project.task_for_task_definition(task_definition) if task.active_overflow_task_claim @@ -35,9 +64,19 @@ class TaskCommentsApi < Grape::API end end - if attached_file.present? - error!({ error: "Attachment is empty." }) if File.size?(attached_file["tempfile"].path).blank? - error!({ error: "Attachment exceeds the maximum attachment size of 30MB." }) unless File.size?(attached_file["tempfile"].path) < 30_000_000 + existing_result = if client_request_id.present? + task.comments.find_by(user_id: current_user.id, client_request_id: client_request_id) + end + + if attached_file.present? && existing_result.blank? + if File.size?(attached_file['tempfile'].path).blank? + FileHelper.log_file_rejection('Attachment is empty', 'comment_attachment', attached_file) + error!({ error: 'Attachment is empty.', code: 'UPLOAD_EMPTY' }, 400) + end + unless File.size?(attached_file['tempfile'].path) < CommentAttachmentPolicy::MAX_BYTES + FileHelper.log_file_rejection('Attachment size limit exceeded', 'comment_attachment', attached_file) + error!({ error: 'Attachment exceeds the maximum attachment size of 30MB.', code: 'UPLOAD_TOO_LARGE' }, 413) + end end type_string = content_type.to_s @@ -48,24 +87,48 @@ class TaskCommentsApi < Grape::API error!(error: 'Original comment is not in this task.') if task.all_comments.find(reply_to_id).blank? end - logger.info("#{current_user.username} - added comment for task #{task.id} (#{task_definition.abbreviation})") - - if attached_file.blank? + if existing_result.present? + result = existing_result + elsif attached_file.blank? error!({ error: 'Comment text is empty, unable to add new comment' }, 403) if text_comment.blank? - result = task.add_text_comment(current_user, text_comment, reply_to_id) + begin + result = task.add_text_comment(current_user, text_comment, reply_to_id, client_request_id) + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e + raise unless lost_client_request_race?(e) + + result = comment_for_client_request(task, client_request_id) + raise if result.blank? + end + result ||= comment_for_client_request(task, client_request_id) else file_result = FileHelper.accept_file(attached_file, 'comment attachment - TaskComment', 'comment_attachment') unless file_result[:accepted] - error!({ error: "File is not an accptable format: #{file_result[:msg]}" }, 403) + error!({ error: "File is not an acceptable format: #{file_result[:msg]}", code: file_result[:code] }, 403) end - result = task.add_comment_with_attachment(current_user, attached_file, reply_to_id) + begin + result = task.add_comment_with_attachment( + current_user, + attached_file, + reply_to_id, + text_comment, + client_request_id + ) + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e + raise unless lost_client_request_race?(e) + + result = comment_for_client_request(task, client_request_id) + raise if result.blank? + end + error!({ error: 'File is not an acceptable comment attachment format.' }, 403) if result.nil? end if result.nil? error!({ error: 'No comment added. Comment duplicates last comment, so ignored.' }, 403) else + logger.info("user_id=#{current_user.id} added comment for task #{task.id} (#{task_definition.abbreviation})") if existing_result.blank? + SessionTracker.record_assessment_activity( action: 'add-comment', user: current_user, @@ -93,20 +156,27 @@ class TaskCommentsApi < Grape::API if project.has_task_for_task_definition? task_definition task = project.task_for_task_definition(task_definition) - comment = task.comments.find(params[:id]) + # all_comments spans the group's shared submission, so a group member can open + # an attachment posted by another member. It stays bounded by this caller's own + # project via the :get check above, matching the delete and update endpoints. + comment = task.all_comments.find(params[:id]) - error!({ error: 'No attachment for this comment.' }, 404) unless %w(audio image pdf).include? comment.content_type + error!({ error: 'No attachment for this comment.' }, 404) unless comment.attachment? error!({ error: 'File missing' }, 404) unless File.exist? comment.attachment_path # Set return content type content_type comment.attachment_mime_type + header['X-Content-Type-Options'] = 'nosniff' env['api.format'] = :binary # mark as attachment - if params[:as_attachment] - header['Content-Disposition'] = "attachment; filename=#{comment.attachment_file_name}" + if params[:as_attachment] || %w[document spreadsheet].include?(comment.content_type) + header['Content-Disposition'] = ActionDispatch::Http::ContentDisposition.format( + disposition: 'attachment', + filename: comment.attachment_file_name + ) end SessionTracker.record_assessment_activity( @@ -118,6 +188,8 @@ class TaskCommentsApi < Grape::API ) stream_file comment.attachment_path + else + error!({ error: 'No attachment for this comment.' }, 404) end end @@ -265,7 +337,9 @@ class TaskCommentsApi < Grape::API task = project.task_for_task_definition(task_definition) - task_comment = task.comments.find(params[:id]) + # Group task feedback is shared across every task in the same group + # submission, matching the collection returned by the comments endpoint. + task_comment = task.all_comments.find(params[:id]) task_comment.mark_as_unread(current_user) SessionTracker.record_assessment_activity( diff --git a/app/api/tasks_api.rb b/app/api/tasks_api.rb index afd9851fe3..b9a7a9cd5d 100644 --- a/app/api/tasks_api.rb +++ b/app/api/tasks_api.rb @@ -67,11 +67,16 @@ class TasksApi < Grape::API end result = base + .preload(:granted_extension_comments, task_definition: :grade_due_dates, project: %i[unit user campus]) .map do |task| { task_definition_id: task.task_definition_id, status: TaskStatus.id_to_key(task.task_status_id), due_date: task.due_date, + effective_deadline: task.effective_deadline, + effective_deadline_date: task.effective_deadline_date, + effective_deadline_reason: task.effective_deadline_reason, + effective_deadline_source_id: task.effective_deadline_source_id, extensions: task.extensions, scorm_extensions: task.scorm_extensions } @@ -168,10 +173,34 @@ class TasksApi < Grape::API # check the user can put this task if authorise? current_user, project, :make_submission + # Only staff who can assess this task may write its grade. This is checked + # before anything below writes, so a refused request leaves the task alone. + if !grade.nil? && !authorise?(current_user, project, :assess) + error!({ error: 'You are not permitted to assess this task' }, 403) + end + task = project.task_for_task_definition(task_definition) + # A tutor can both mark and unmark a task as discussed in class. Sending + # discussed:false used to still add a "Discussed in class" comment, the + # opposite of what it asks, and that comment type cannot be removed through + # the UI. So false now removes all discussed markers instead. + # The mark is added here so a same-request complete trigger below can see it; + # a removal is deferred to the end so a later refused trigger or grade does + # not leave the comment destroyed and the request still failing. + remove_discussed = false if !params[:discussed].nil? && authorise?(current_user, project, :assess) - task.add_discussed_comment(current_user) + if params[:discussed] + task.add_discussed_comment(current_user) + elsif task.task_definition.requires_discussion && + (task.task_status == TaskStatus.complete || params[:trigger] == 'complete') + # Removing the mark would leave a discussion-required task complete + # without the evidence the model demands. Refuse before deleting + # anything. + error!({ error: 'Cannot remove the discussed mark from a task that requires discussion while it is complete. Change its status first.' }, 403) + else + remove_discussed = true + end end # if trigger supplied... @@ -211,11 +240,18 @@ class TasksApi < Grape::API recursive_fix: params[:trigger_recursive_fix], check_feedback: true ) - if result.nil? && task.errors.any? - error!({ error: task.errors.full_messages.to_sentence }, 403) - end - if result.nil? && task.task_definition.restrict_status_updates - error!({ error: 'This task can only be updated by your tutor.' }, 403) + # trigger_transition returns nil for every refusal, and most of its early + # returns leave errors empty. Both guards below used to need something + # extra on top of that, so a refused change fell through to the 200 at the + # end of the handler and the client showed it as accepted. + if result.nil? + if task.errors.any? + error!({ error: task.errors.full_messages.to_sentence }, 403) + elsif task.task_definition.restrict_status_updates + error!({ error: 'This task can only be updated by your tutor.' }, 403) + else + error!({ error: 'This status change is not allowed for this task.' }, 403) + end end SessionTracker.record_assessment_activity( action: "assessing", @@ -238,6 +274,10 @@ class TasksApi < Grape::API task.save end + # The status change and grade have been applied without error, so it is now + # safe to remove the discussed mark that was requested with discussed:false. + task.remove_discussed_comment if remove_discussed + present task, with: Entities::TaskEntity, include_other_projects: true, update_only: true else error!({ error: "Couldn't find Task with id=#{params[:id]}" }, 403) @@ -269,7 +309,7 @@ class TasksApi < Grape::API task_definition = project.unit.task_definitions.find(params[:task_definition_id]) # check the user can put this task - error!(error: 'You do not have permission to read submissions for this project.') unless authorise? current_user, project, :get_submission + error!({ error: 'You do not have permission to read submissions for this project.' }, 403) unless authorise? current_user, project, :get_submission # ensure there can be a pdf... needs_upload_docs = !task_definition.upload_requirements.empty? @@ -283,17 +323,21 @@ class TasksApi < Grape::API end result = if needs_upload_docs && task - # return the details as json - { - has_pdf: task.has_pdf, + task.submission_processing_snapshot.merge( submission_date: task.submission_date, - processing_pdf: task.processing_pdf?, + processing_error_code: task.submission_processing_error_code, + processing_attempts: task.submission_processing_attempts, task_status: task.task_status.status_key - } + ) else { has_pdf: false, - processing_pdf: false + pdf_ready: false, + submission_files_ready: false, + processing_pdf: false, + processing_state: 'not_submitted', + retryable: false, + poll_after_seconds: nil } end @@ -324,7 +368,7 @@ class TasksApi < Grape::API task_definition = project.unit.task_definitions.find(params[:task_definition_id]) # check the user can put this task - error!(error: 'You do not have permission to read submissions for this project.') unless authorise? current_user, project, :get_submission + error!({ error: 'You do not have permission to read submissions for this project.' }, 403) unless authorise? current_user, project, :get_submission # Get the actual task... task = project.task_for_task_definition(task_definition) diff --git a/app/api/users_api.rb b/app/api/users_api.rb index 2900bbfba4..b40943961c 100644 --- a/app/api/users_api.rb +++ b/app/api/users_api.rb @@ -1,6 +1,7 @@ require 'grape' class UsersApi < Grape::API + helpers CollectionPaginationHelpers helpers AuthenticationHelpers helpers AuthorisationHelpers helpers MimeCheckHelpers @@ -10,12 +11,17 @@ class UsersApi < Grape::API end desc 'Get the list of users' + params do + optional :page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PAGE, allow_blank: false + optional :per_page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PER_PAGE, allow_blank: false + end get '/users' do unless authorise? current_user, User, :list_users error!({ error: 'Cannot list users - not authorised' }, 403) end - present User.all.eager_load(:role), with: Entities::UserEntity + users = paginate_collection(User.eager_load(:role)) + present users, with: Entities::UserEntity end desc 'Get user' @@ -25,25 +31,37 @@ class UsersApi < Grape::API error!({ error: "Cannot find User with id #{params[:id]}" }, 403) end - present user, with: Entities::UserEntity + present user, + with: Entities::UserEntity, + theme_owner_id: current_user.id end desc 'Get convenors' + params do + optional :page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PAGE, allow_blank: false + optional :per_page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PER_PAGE, allow_blank: false + end get '/users/convenors' do unless authorise? current_user, User, :get_staff_list error!({ error: 'Cannot list convenors - not authorised' }, 403) end - present User.convenors, with: Entities::UserEntity + users = paginate_collection(User.convenors.eager_load(:role)) + present users, with: Entities::UserEntity end desc 'Get tutors' + params do + optional :page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PAGE, allow_blank: false + optional :per_page, type: Integer, values: 1..CollectionPaginationHelpers::MAX_PER_PAGE, allow_blank: false + end get '/users/tutors' do unless authorise? current_user, User, :get_staff_list error!({ error: 'Cannot list tutors - not authorised' }, 403) end - present User.tutors.eager_load(:role), with: Entities::UserEntity + users = paginate_collection(User.tutors.eager_load(:role)) + present users, with: Entities::UserEntity end desc 'Update a user' @@ -59,16 +77,25 @@ class UsersApi < Grape::API optional :receive_task_notifications, type: Boolean, desc: 'Allow user to be sent task notifications' optional :receive_portfolio_notifications, type: Boolean, desc: 'Allow user to be sent portfolio notifications' optional :receive_feedback_notifications, type: Boolean, desc: 'Allow user to be sent feedback notifications' + optional :display_peer_progress, type: Boolean, desc: 'Display anonymous peer progress information' optional :opt_in_to_research, type: Boolean, desc: 'Allow user to opt in to research conducted by Doubtfire' optional :has_run_first_time_setup, type: Boolean, desc: 'Whether or not user has run first-time setup' + optional :theme_preference, type: String, desc: 'Theme preference for the user [light, dark, system]; null means never chosen' end end put '/users/:id' do change_self = (params[:id] == current_user.id) - params[:receive_portfolio_notifications] = true if params.key?(:receive_portfolio_notifications) && params[:receive_portfolio_notifications].nil? - params[:receive_portfolio_notifications] = true if params.key?(:receive_feedback_notifications) && params[:receive_feedback_notifications].nil? - params[:receive_portfolio_notifications] = true if params.key?(:receive_task_notifications) && params[:receive_task_notifications].nil? + # Default notification preferences to true when explicitly sent as null. + # (Previously this wrote the portfolio key three times and read the + # top-level params instead of the nested :user hash, so it never applied.) + %i[receive_task_notifications receive_portfolio_notifications receive_feedback_notifications].each do |pref| + params[:user][pref] = true if params[:user].key?(pref) && params[:user][pref].nil? + end + if params[:user].key?(:display_peer_progress) && + params[:user][:display_peer_progress].nil? + params[:user][:display_peer_progress] = true + end # can only modify if current_user.id is same as :id provided # (i.e., user wants to update their own data) or if update_user token @@ -76,6 +103,23 @@ class UsersApi < Grape::API user = User.eager_load(:role).find(params[:id]) + # Identity asserted by SAML/AAF/LDAP is refreshed by the institution's + # sign-in/import path. It must not be forgeable through the profile API, + # including by an administrator. Student ids are also account data, not + # a self-service profile field. Local database-auth administrators retain + # the existing ability to maintain another account's identity. + if params[:user].key?(:email) && + params[:user][:email].to_s != user.email.to_s && + !AuthenticationHelpers.db_auth? + error!({ error: 'Sign-in email is managed by your institution and cannot be changed here.' }, 422) + end + + if params[:user].key?(:student_id) && + params[:user][:student_id].to_s != user.student_id.to_s && + (change_self || !AuthenticationHelpers.db_auth?) + error!({ error: 'Student ID is managed account information and cannot be changed here.' }, 422) + end + user_parameters = ActionController::Parameters.new(params) .require(:user) .permit( @@ -87,10 +131,17 @@ class UsersApi < Grape::API :receive_task_notifications, :receive_portfolio_notifications, :receive_feedback_notifications, + :display_peer_progress, :opt_in_to_research, - :has_run_first_time_setup + :has_run_first_time_setup, + :theme_preference ) + # Theme preference belongs only to the account itself. Keep authorised + # staff profile updates backward-compatible by ignoring this one private + # field instead of rejecting the rest of an otherwise valid update. + user_parameters.delete(:theme_preference) unless change_self + user.role = Role.student if user.role.nil? old_role = user.role @@ -115,13 +166,13 @@ class UsersApi < Grape::API error!({ error: "No such role name #{user_parameters[:role]}" }, 403) end - if old_role == Role.auditor - action - :promote_user - elsif new_role == Role.auditor - action = old_role == Role.student ? :promote_user : :demote_user - else - action = new_role.id > old_role.id ? :promote_user : :demote_user - end + action = if old_role == Role.auditor + :promote_user + elsif new_role == Role.auditor + old_role == Role.student ? :promote_user : :demote_user + else + new_role.id > old_role.id ? :promote_user : :demote_user + end # current user not authorised to peform action with new role? unless authorise? current_user, User, action, User.get_change_role_perm_fn, [old_role.to_sym, new_role.to_sym] @@ -131,9 +182,18 @@ class UsersApi < Grape::API user_parameters[:role] = new_role end + # An explicit preference is a synchronization write, even when its value + # matches the stored value. Clients use this timestamp to reconcile a + # newer offline choice with the account copy. + if user_parameters.key?(:theme_preference) + user.theme_preference_updated_at = user_parameters[:theme_preference].nil? ? nil : Time.current + end + # Update changes made to user user.update!(user_parameters) - present user, with: Entities::UserEntity + present user, + with: Entities::UserEntity, + theme_owner_id: current_user.id else error!({ error: "Cannot modify user with id=#{params[:id]} - not authorised" }, 403) end diff --git a/app/controllers/readiness_controller.rb b/app/controllers/readiness_controller.rb new file mode 100644 index 0000000000..d23bce182e --- /dev/null +++ b/app/controllers/readiness_controller.rb @@ -0,0 +1,5 @@ +class ReadinessController < ActionController::API + def show + head(ReadinessCheck.new.ready? ? :ok : :service_unavailable) + end +end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c74883d014..5108dc66ea 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -17,6 +17,11 @@ class TaskComment < ApplicationRecord has_many :comments_read_receipts, class_name: 'CommentsReadReceipts', dependent: :destroy, inverse_of: :task_comment + # Notifications that point at this comment. A deleted comment can never turn + # up in the set handed to Task#mark_comments_as_read, so a notification left + # behind here could never be cleared by reading. It goes with the comment. + has_many :notifications, as: :notifiable, dependent: :destroy, inverse_of: :notifiable + # Can optionally be a reply to a comment belongs_to :task_comment, optional: true @@ -27,6 +32,11 @@ class TaskComment < ApplicationRecord validates :user, presence: true validates :recipient, presence: true validates :comment, length: { minimum: 0, maximum: 4095, allow_blank: true } + validates :client_request_id, + length: { maximum: 64 }, + format: { with: /\A[0-9a-f-]+\z/i }, + uniqueness: { scope: [:user_id, :task_id] }, + allow_blank: true validate :valid_reply_to?, on: :create # After create, mark as read by user creating @@ -52,10 +62,10 @@ def delete_associated_files end def serialize(user) - { + result = { id: self.id, comment: self.comment, - has_attachment: ["audio", "image", "pdf"].include?(self.content_type), + has_attachment: attachment?, type: self.content_type || "text", is_new: self.new_for?(user), reply_to_id: self.reply_to_id, @@ -63,17 +73,27 @@ def serialize(user) id: self.user.id, first_name: self.user.first_name, last_name: self.user.last_name, + display_name: self.user.display_name, email: self.user.email }, recipient: { id: self.recipient.id, first_name: self.recipient.first_name, last_name: self.recipient.last_name, + display_name: self.recipient.display_name, email: self.recipient.email }, created_at: self.created_at, recipient_read_time: self.time_read_by(self.recipient), } + + if attachment? + result[:attachment_file_name] = attachment_file_name + result[:attachment_mime_type] = attachment_mime_type + result[:attachment_byte_size] = attachment_size + end + + result end def create_comment_read_receipt_entry(user) @@ -81,20 +101,35 @@ def create_comment_read_receipt_entry(user) end def comment + stored_comment = super + return stored_comment if stored_comment.present? + return 'audio comment' if content_type == 'audio' return 'image comment' if content_type == 'image' return 'pdf document' if content_type == 'pdf' + return 'document attachment' if content_type == 'document' + return 'spreadsheet attachment' if content_type == 'spreadsheet' return 'discussion comment' if content_type == 'discussion' - super + stored_comment + end + + def attachment? + %w[audio image pdf document spreadsheet].include?(content_type) && attachment_extension.present? end def attachment_path FileHelper.comment_attachment_path(self, attachment_extension) end + # The supplied name, with the stored extension when processing changed the + # format (audio is stored as WAV, most images as JPEG). def attachment_file_name - "comment-#{id}#{attachment_extension}" + original = attachment_original_filename.presence + return "comment-#{id}#{attachment_extension}" if original.nil? + return original if attachment_extension.blank? || File.extname(original).casecmp?(attachment_extension) + + "#{File.basename(original, File.extname(original))}#{attachment_extension}" end def add_attachment(file_upload) @@ -113,29 +148,51 @@ def add_attachment(file_upload) '.jpg' end save - FileHelper.compress_image_to_dest(file_upload["tempfile"].path, attachment_path) - else + return false unless FileHelper.compress_image_to_dest(file_upload["tempfile"].path, attachment_path) + elsif content_type == 'pdf' self.attachment_extension = '.pdf' save FileHelper.compress_pdf(file_upload["tempfile"].path) FileUtils.mv file_upload["tempfile"].path, attachment_path + elsif %w[document spreadsheet].include?(content_type) + self.attachment_extension = File.extname(file_upload['filename'] || file_upload[:filename]).downcase + save + FileUtils.mv file_upload["tempfile"].path, attachment_path + else + return false end - file_upload["tempfile"].unlink + return false unless File.file?(attachment_path) && File.size?(attachment_path).present? + + self.attachment_original_filename = FileHelper.safe_upload_filename( + file_upload, + fallback: "comment-#{id}#{attachment_extension}" + ) + self.attachment_content_type = mime_type(attachment_path).to_s.split(';').first + self.attachment_byte_size = File.size(attachment_path) + save! + + file_upload["tempfile"].unlink if File.exist?(file_upload["tempfile"].path) true end def attachment_mime_type - if attachment_extension == '.wav' + if attachment_content_type.present? + attachment_content_type + elsif attachment_extension == '.wav' 'audio/wav; charset:binary' else mime_type(attachment_path) end end + def attachment_size + attachment_byte_size || (File.size(attachment_path) if File.exist?(attachment_path)) + end + def remove_comment_read_entry(user) - CommentsReadReceipts.delete_all(user: user, task_comment: self) + CommentsReadReceipts.where(user: user, task_comment: self).delete_all end def mark_as_read(user, unit = self.unit) diff --git a/app/models/courseflow/course.rb b/app/models/courseflow/course.rb new file mode 100644 index 0000000000..8fa93fbdbb --- /dev/null +++ b/app/models/courseflow/course.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +module Courseflow + # A versioned planning catalog, independent of teaching units and enrolments. + class Course < ApplicationRecord + self.table_name = 'courseflow_courses' + attribute :units, :json + + MAX_UNITS = 240 + UNIT_KEYS = %w[code name required prerequisites offered_trimesters].freeze + CATALOG_KEYS = %w[code name version elective_count units].freeze + UNIT_CODE = /\A[A-Z0-9][A-Z0-9_-]{0,19}\z/ + + has_many :maps, class_name: 'Courseflow::CourseMap', dependent: :restrict_with_exception + + validates :code, presence: true, length: { maximum: 40 }, format: { with: /\A[A-Z0-9][A-Z0-9_-]*\z/ } + validates :name, presence: true, length: { maximum: 200 } + validates :version, presence: true, length: { maximum: 40 }, uniqueness: { scope: :code } + validates :elective_count, numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validate :validate_units + validate :immutable_catalog, on: :update + + def as_catalog + attributes.slice('id', *CATALOG_KEYS) + end + + private + + def immutable_catalog + errors.add(:base, 'Catalog versions are immutable; import a new version') if changed.intersect?(CATALOG_KEYS) + end + + def validate_units + unless units.is_a?(Array) && units.length.between?(1, MAX_UNITS) + errors.add(:units, "must contain between 1 and #{MAX_UNITS} unit definitions") + return + end + + units.each_with_index { |unit, index| validate_unit(unit, index) } + return if errors[:units].any? + + codes = units.pluck('code') + errors.add(:units, 'must have unique codes') if codes.uniq.length != codes.length + units.each do |unit| + errors.add(:units, "#{unit['code']} has unknown prerequisites") if (unit['prerequisites'] - codes).any? + end + return if errors[:units].any? + + dependencies = units.to_h { |unit| [unit['code'], unit['prerequisites']] } + if cyclic?(dependencies) + errors.add(:units, 'prerequisites must not contain cycles') + return + end + validate_elective_count(dependencies) + end + + def validate_unit(unit, index) + unless unit.is_a?(Hash) && unit.keys.sort == UNIT_KEYS.sort + errors.add(:units, "entry #{index + 1} must contain exactly #{UNIT_KEYS.join(', ')}") + return + end + + valid = unit['code'].is_a?(String) && UNIT_CODE.match?(unit['code']) && + unit['name'].is_a?(String) && unit['name'].strip.present? && unit['name'].length <= 200 && + [true, false].include?(unit['required']) && + unit['prerequisites'].is_a?(Array) && unit['prerequisites'].length <= MAX_UNITS && + unit['prerequisites'].all? { |code| code.is_a?(String) && UNIT_CODE.match?(code) } && + unit['prerequisites'].uniq == unit['prerequisites'] && + unit['offered_trimesters'].is_a?(Array) && unit['offered_trimesters'].any? && + unit['offered_trimesters'].all? { |value| value.is_a?(Integer) && value.between?(1, 3) } && + unit['offered_trimesters'].uniq == unit['offered_trimesters'] + errors.add(:units, "entry #{index + 1} has invalid planning rules") unless valid + end + + def cyclic?(dependencies) + visiting = Set.new + visited = Set.new + visit = lambda do |code| + return true if visiting.include?(code) + return false if visited.include?(code) + + visiting.add(code) + return true if dependencies.fetch(code).any? { |prerequisite| visit.call(prerequisite) } + + visiting.delete(code) + visited.add(code) + false + end + dependencies.keys.any? { |code| visit.call(code) } + end + + def validate_elective_count(dependencies) + return unless elective_count.is_a?(Integer) + + required = units.select { |unit| unit['required'] }.pluck('code') + closure = required.to_set + pending = required.dup + until pending.empty? + dependencies.fetch(pending.pop).each do |prerequisite| + pending << prerequisite if closure.add?(prerequisite) + end + end + minimum_electives = closure.length - required.length + maximum_electives = units.length - required.length + depths = {} + depth = ->(code) { depths[code] ||= 1 + (dependencies.fetch(code).map { |prerequisite| depth.call(prerequisite) }.max || 0) } + if required.any? { |code| depth.call(code) > 60 } + errors.add(:units, 'required prerequisite chains must fit within 60 study periods') + end + return if elective_count.between?(minimum_electives, maximum_electives) + + errors.add(:elective_count, "must be between #{minimum_electives} and #{maximum_electives} for these prerequisites") + end + end +end diff --git a/app/models/courseflow/course_map.rb b/app/models/courseflow/course_map.rb new file mode 100644 index 0000000000..7f75892ed7 --- /dev/null +++ b/app/models/courseflow/course_map.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +module Courseflow + # A student's complete plan is one row, so failed saves cannot leave half a plan. + class CourseMap < ApplicationRecord + self.table_name = 'courseflow_maps' + attribute :periods, :json + attribute :slots, :json + + PERIOD_KEYS = %w[year trimester].freeze + SLOT_KEYS = %w[unit_code year trimester position].freeze + + belongs_to :user + belongs_to :course, class_name: 'Courseflow::Course' + + validates :name, presence: true, length: { maximum: 200 } + validate :immutable_references, on: :update + validate :validate_plan + + def as_plan + planning_issues = issues + attributes.slice('id', 'course_id', 'name', 'lock_version', 'periods', 'slots').merge( + 'issues' => planning_issues, + 'complete' => planning_issues.empty?, + 'updated_at' => updated_at.iso8601(6) + ) + end + + # These are planning checks only, not an assessment of degree eligibility. + def issues + scheduled = slots.index_by { |slot| slot['unit_code'] } + result = [] + course.units.each do |unit| + slot = scheduled[unit['code']] + if unit['required'] && slot.nil? + result << issue('missing_required', "Add required unit #{unit['code']}.", unit['code']) + end + next unless slot + + unless unit['offered_trimesters'].include?(slot['trimester']) + result << issue('unavailable_trimester', "#{unit['code']} is not offered in trimester #{slot['trimester']}.", unit['code']) + end + unit['prerequisites'].each do |prerequisite| + preceding = scheduled[prerequisite] + next if preceding && period_number(preceding) < period_number(slot) + + result << issue('prerequisite', "Schedule #{prerequisite} before #{unit['code']}.", unit['code']) + end + end + elective_codes = course.units.reject { |unit| unit['required'] }.pluck('code') + elective_total = (scheduled.keys & elective_codes).length + if elective_total != course.elective_count + result << issue('elective_count', "Plan exactly #{course.elective_count} elective units; currently #{elective_total}.") + end + result + end + + private + + def issue(code, message, unit_code = nil) + { 'code' => code, 'message' => message }.tap { |value| value['unit_code'] = unit_code if unit_code } + end + + def period_number(value) + (value['year'] * 3) + value['trimester'] + end + + def immutable_references + errors.add(:course_id, 'cannot change on a saved map') if will_save_change_to_course_id? + errors.add(:user_id, 'cannot change on a saved map') if will_save_change_to_user_id? + end + + def valid_period?(period) + period.is_a?(Hash) && period['year'].is_a?(Integer) && period['year'].between?(2000, 2200) && + period['trimester'].is_a?(Integer) && period['trimester'].between?(1, 3) + end + + def validate_plan + unless periods.is_a?(Array) && periods.length.between?(1, 60) && + periods.all? { |period| valid_period?(period) && period.keys.sort == PERIOD_KEYS.sort } + errors.add(:periods, 'must contain 1 to 60 periods with integer year 2000..2200 and trimester 1..3') + return + end + period_ids = periods.map { |period| period_number(period) } + errors.add(:periods, 'must not contain duplicates') if period_ids.uniq.length != period_ids.length + + unless slots.is_a?(Array) && slots.length <= 240 && slots.all? { |slot| valid_slot?(slot) } + errors.add(:slots, 'must contain at most 240 slots with a unit code and integer year, trimester and position 1..4') + return + end + validate_slot_references(period_ids) + end + + def valid_slot?(slot) + valid_period?(slot) && slot.keys.sort == SLOT_KEYS.sort && + slot['unit_code'].is_a?(String) && Course::UNIT_CODE.match?(slot['unit_code']) && + slot['position'].is_a?(Integer) && slot['position'].between?(1, 4) + end + + def validate_slot_references(period_ids) + codes = slots.pluck('unit_code') + positions = slots.map { |slot| [period_number(slot), slot['position']] } + errors.add(:slots, 'must not repeat a unit') if codes.uniq.length != codes.length + errors.add(:slots, 'must not repeat a position') if positions.uniq.length != positions.length + errors.add(:slots, 'must belong to a declared period') unless slots.all? { |slot| period_ids.include?(period_number(slot)) } + return unless course + + errors.add(:slots, 'must use unit codes from the selected course') if (codes - course.units.pluck('code')).any? + end + end +end diff --git a/app/models/unit.rb b/app/models/unit.rb index 19e0098298..0a92a99035 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -79,6 +79,7 @@ def self.permissions :upload_grades_csv, :get_staff_notes, :capture_task_completion_snapshot, + :run_similarity_scan, :mannage_communications, :delete_engagement ] @@ -108,6 +109,7 @@ def self.permissions :get_marking_sessions, :get_staff_notes, :get_tutor_times, + :run_similarity_scan, :mannage_communications, ] @@ -170,10 +172,14 @@ def role_for(user) has_many :task_definitions, dependent: :destroy, inverse_of: :unit has_many :tutorials, dependent: :destroy, inverse_of: :unit # tutorials need groups and tasks deleted before it... has_many :tutorial_streams, dependent: :destroy, inverse_of: :unit + has_many :teams_announcement_sync_states, dependent: :destroy + has_many :unit_announcements, dependent: :destroy, inverse_of: :unit + has_many :unit_learning_sessions, dependent: :destroy, inverse_of: :unit has_many :unit_roles, dependent: :destroy, inverse_of: :unit has_many :learning_outcomes, as: :context, dependent: :destroy # inverse_of: :unit has_many :marking_sessions, dependent: :destroy has_many :task_completion_snapshots, dependent: :destroy, inverse_of: :unit + has_many :peer_progress_snapshots, dependent: :destroy, inverse_of: :unit has_many :communication_sets, class_name: 'CommunicationSet', dependent: :destroy has_many :communication_rules, through: :communication_sets, class_name: 'CommunicationRule' has_many :communication_set_schedules, through: :communication_sets, class_name: 'CommunicationSetSchedule' @@ -269,7 +275,15 @@ def saved_change_to_communication_schedule_inputs? end def ordered_task_definitions - task_definitions.order('start_date ASC, abbreviation ASC') + return task_definitions.order('start_date ASC, abbreviation ASC') unless task_definitions.loaded? + + task_definitions.sort_by do |task_definition| + [ + task_definition.start_date.nil? ? 0 : 1, + task_definition.start_date, + task_definition.abbreviation.to_s + ] + end end def convenors @@ -489,6 +503,7 @@ def autogen_date_within_unit_active_period def rollover(teaching_period, start_date, end_date, new_code) new_unit = self.dup + copied_task_definitions = [] new_unit.code = new_code if new_code.present? @@ -541,6 +556,7 @@ def rollover(teaching_period, start_date, end_date, new_code) # Duplicate task definitions task_definitions.each do |td| new_td = td.copy_to(new_unit) + copied_task_definitions << new_td td.learning_outcomes.each do |learning_outcome| # for each old task definition, duplicate the learning outcomes associated with it aswell new_outcome = learning_outcome.dup @@ -611,6 +627,8 @@ def rollover(teaching_period, start_date, end_date, new_code) end end + NewTaskAvailableNotificationJob.track_and_enqueue_all(copied_task_definitions) + new_unit end @@ -1374,7 +1392,7 @@ def unenrol_users_from_csv(file) if user_project.enrolled user_project.enrolled = false user_project.save - success << { row: row, message: "User #{username} withdrawn from unit" } + success << { row: row, project_id: user_project.id, message: "User #{username} withdrawn from unit" } else ignored << { row: row, message: "User #{username} not enrolled in unit" } end @@ -1633,7 +1651,9 @@ def import_student_groups_from_csv(group_set, file) project.enrol_in(grp.tutorial) end - grp.add_member(project) + # Bulk imports can add many students in one request. Do not send a + # separate notification for every CSV row. + grp.add_member(project, notify: false) success << { row: row, message: "Added #{username} to #{grp.name}." } rescue Exception => e @@ -1705,8 +1725,10 @@ def date_for_week_and_day(week, day) return nil if day_num.nil? start_day_num = start_date.wday + day_offset = day_num - start_day_num + day_offset += 7 if day_offset.negative? - start_date + (week - 1).weeks + (day_num - start_day_num).days + start_date + (week - 1).weeks + day_offset.days end end @@ -1722,10 +1744,11 @@ def week_number(date) end end - def import_tasks_from_csv(file) + def import_tasks_from_csv(file, notify: true) success = [] errors = [] ignored = [] + imported_task_definitions = [] data = read_file_to_str(file) @@ -1756,6 +1779,7 @@ def import_tasks_from_csv(file) end prerequisites_by_task[task_definition.abbreviation] = JSON.parse(row[:task_prerequisites]) unless row[:task_prerequisites].nil? + imported_task_definitions << task_definition if new_task success << { row: row, message: message } rescue Exception => e @@ -1797,6 +1821,10 @@ def import_tasks_from_csv(file) end end + if notify + NewTaskAvailableNotificationJob.track_and_enqueue_all(imported_task_definitions) + end + { success: success, ignored: ignored, diff --git a/app/services/courseflow/catalog_importer.rb b/app/services/courseflow/catalog_importer.rb new file mode 100644 index 0000000000..68cdc49ca1 --- /dev/null +++ b/app/services/courseflow/catalog_importer.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module Courseflow + # Administrative import only. New curricula always have a new version, even + # before a student saves a plan, avoiding catalog/map creation races. + class CatalogImporter + MAX_BYTES = 1_048_576 + + def self.import_file!(path) + document = File.open(path, 'rb') { |file| file.read(MAX_BYTES + 1) } + raise ArgumentError, 'Catalog must be at most 1 MiB' if document.bytesize > MAX_BYTES + + import!(JSON.parse(document)) + end + + def self.import!(document) + validate_shape!(document) + candidate = Course.new(document) + Course.transaction do + existing = Course.find_by(code: candidate.code, version: candidate.version) + if existing + raise ArgumentError, 'Catalog version already exists with different data; use a new version' unless existing.as_catalog.except('id') == document + + existing + else + candidate.save! + candidate + end + end + end + + def self.validate_shape!(document) + unless document.is_a?(Hash) && document.keys.sort == Course::CATALOG_KEYS.sort + raise ArgumentError, "Catalog must contain exactly #{Course::CATALOG_KEYS.join(', ')}" + end + unless %w[code name version].all? { |field| document[field].is_a?(String) && document[field].strip.present? } && + document['elective_count'].is_a?(Integer) + raise ArgumentError, 'Catalog code, name and version must be nonblank strings; elective_count must be an integer' + end + end + end +end diff --git a/app/services/readiness_check.rb b/app/services/readiness_check.rb new file mode 100644 index 0000000000..6d25f41251 --- /dev/null +++ b/app/services/readiness_check.rb @@ -0,0 +1,28 @@ +class ReadinessCheck + DATABASE_QUERY = 'SELECT 1'.freeze + + def initialize(database_connection_pool: ActiveRecord::Base.connection_pool, redis: Sidekiq) + @database_connection_pool = database_connection_pool + @redis = redis + end + + def ready? + database_ready? && redis_ready? + rescue StandardError + false + end + + private + + def database_ready? + result = @database_connection_pool.with_connection do |connection| + connection.select_value(DATABASE_QUERY) + end + + result.to_s == '1' + end + + def redis_ready? + @redis.redis(&:ping) == 'PONG' + end +end diff --git a/app/sidekiq/check_unit_similarity_job.rb b/app/sidekiq/check_unit_similarity_job.rb new file mode 100644 index 0000000000..4edfb36259 --- /dev/null +++ b/app/sidekiq/check_unit_similarity_job.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +# On-demand plagiarism rescan for a single unit. Wraps Unit#check_jplag_similarity +# so a convenor can trigger a scan from the product instead of waiting for the +# nightly cron, which is the only thing that ran it before. +# +# Locked until_executed and rejecting on conflict, keyed on the unit id, so two +# convenors pressing the button cannot queue duplicate scans for the same unit. +class CheckUnitSimilarityJob + include Sidekiq::Job + include Sidekiq::Status::Worker + include LogHelper + include ApplicationHelper + + sidekiq_options lock: :until_executed, + lock_args_method: ->(args) { [args.first] }, + on_conflict: :reject, + retry: false + + # Two entry points, both keyed on the unit id so a nightly scan and an on-demand + # scan of the same unit reject each other rather than racing on the shared + # tmp/jplag working directory. + # + # - No unit id: the config/schedule.yml cron enqueues this way. It fans out one + # child job per active unit with force off, so only units whose files or task + # definitions changed are rescanned, and one unit's failure does not abort the + # rest. Each child locks on its own unit id. + # - A unit id: one unit is scanned. The endpoint passes force true so a threshold + # change alone is enough to rescan, which is the gap this path exists to close; + # the nightly children pass force false. + # + # task_definition_id is accepted so the queued job and its lock key are stable if + # per-definition scanning is added later. check_jplag_similarity is unit-scoped + # today, so the scan currently covers the whole unit regardless. + def perform(unit_id = nil, force = nil, task_definition_id = nil) + at(0) + total(1) + + if unit_id.present? + logger.info "Starting similarity scan for unit #{unit_id} (force=#{force})..." + if task_definition_id.present? + logger.info "Similarity scan requested for task definition #{task_definition_id}; " \ + "running a unit-wide scan because check_jplag_similarity is unit-scoped." + end + Unit.find(unit_id).check_jplag_similarity(force: force) + else + logger.info 'Fanning out nightly similarity scans for active units...' + Unit.active_units.find_each { |unit| CheckUnitSimilarityJob.perform_async(unit.id, false) } + end + + at(1) + logger.info 'Completed similarity scan dispatch!' + rescue StandardError => e + logger.error e + raise e + end +end diff --git a/db/migrate/20260830063140_add_theme_preference_to_users.rb b/db/migrate/20260830063140_add_theme_preference_to_users.rb new file mode 100644 index 0000000000..5f37374e95 --- /dev/null +++ b/db/migrate/20260830063140_add_theme_preference_to_users.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddThemePreferenceToUsers < ActiveRecord::Migration[8.0] + def change + add_column :users, :theme_preference, :string + add_column :users, :theme_preference_updated_at, :datetime + end +end diff --git a/docs/display-name-migration.md b/docs/display-name-migration.md new file mode 100644 index 0000000000..5131680e37 --- /dev/null +++ b/docs/display-name-migration.md @@ -0,0 +1,113 @@ +# Display Name Migration + +## Canonical rule + +Normal user-facing display names use: + +- preferred/nickname first name + surname when a preferred name exists +- legal first name + surname when no preferred name exists + +The API is the source of truth for this rule. + +Both parts are trimmed. A missing, empty or whitespace-only nickname falls back +to the trimmed legal first name. Empty parts are omitted and the remaining parts +are joined with one space. Preserve spelling and Unicode; do not title-case, +truncate or append the legal first name to the display value. A display name is +not unique: use the user ID for associations and actions, and an existing +authorised identifier when a staff member needs to distinguish namesakes. + +Use `User#display_name` for a user instance or `User.display_name_for` when working with name attributes outside a full user instance. + +API entities expose this value as: + +`display_name` + +The Web application maps this to: + +`displayName` + +## Contributor guidance + +New user-facing UI should use `displayName` rather than reconstructing a name from `firstName`, `lastName`, `nickname`, `preferredName`, or the legacy `name` property. + +Do not add new frontend fallback logic for normal display names. + +`User#name` and the Web `User.name` property remain legacy behaviour and were not globally changed in MISC-PN02 to avoid an unrelated broad refactor. + +## Priority surfaces migrated in MISC-PN02 + +The scoped migration covers priority user-facing areas including: + +- task comments and comment-related views +- task and staff views +- dashboards and progress views +- student and staff lists +- student-list CSV export +- search display matching +- selected notification/confirmation text + +## Known lower-priority surfaces not migrated + +The following areas still contain legacy name construction and should use the shared display-name value when they are migrated in future work: + +- group management and group alerts +- portfolio/review screens +- SCORM learner-name integration +- similarity/footer display +- QR/header display +- tutorial and campus alerts +- legacy `User.name` +- comment initials generation + +These are deferred migration surfaces, not approved legal-name exceptions. + +## Legal-name exceptions and access + +This is the proposed shared contract for MISC-PN01 review. The normal display +rule above is already implemented. Repository review does not establish an +institution's legal or administrative requirement; the institution's privacy +and administration owners must approve any exception before it is introduced. + +| Purpose | Name to display | Access and exception rule | +| --- | --- | --- | +| Student/staff screens, feedback authors, dashboards, notifications and ordinary class lists | `display_name` / `displayName` | Existing endpoint and unit membership authorisation still applies. Do not add a second legal-name label. | +| A person editing their own identity/profile | Distinct, explicitly labelled editable fields | Only the person and existing authorised administrators. A profile field is not a reason to expose both names elsewhere. | +| An identity check, statutory record or an approved official export | Legal first name and surname only when the receiving process requires them | Record the purpose, responsible institutional owner, allowed roles, receiving system and retention requirements in the change's review. Do not infer permission merely because a user is staff. | +| Ordinary CSV/class-list export | Display name | The current student-list export uses the shared helper. Retaining separate legal-name columns in an administrative import/export requires the documented exception above. | + +No new legal-name exception is approved by this document. Existing portfolio, +SCORM and other legacy name construction below must be reviewed on its actual +purpose; its current implementation is not evidence of an approved exception. + +## Search and privacy + +Normal result labels use the canonical display name. Search may match preferred +name, legal first name, surname and an authorised identifier only within the +records and fields the caller is already allowed to access. A match must not +reveal the hidden matching legal name in a tooltip, highlighted snippet or +secondary label. Search must never expand a student request into a global user +directory. The current Web `User.matches` searches already-loaded user records; +it is not an access-control boundary. + +Do not attach both names to push payloads, email subjects, public links or logs +to disambiguate people. Keep full feedback text and names out of dashboard +metadata. Namesakes must not be merged or treated as the same account. + +## Source audit and next migration boundaries + +Reviewed against API `bb360dfa` and Web `d16f6201c` (`11.0.x`). This is a code +audit, not a claim of institutional approval or a live-user test. + +| Surface | Current source and behaviour | Follow-up priority | +| --- | --- | --- | +| Shared API and Web model | `app/models/user.rb` exposes `display_name_for`/`display_name`; `app/api/entities/user_entity.rb` exposes it; Web `src/app/api/models/user/user.ts` stores `displayName` | Use this contract for new code; retain the existing helper tests. | +| Feedback author display | Web `src/app/tasks/task-comments-viewer/task-comments-viewer.component.html` uses `comment.author.displayName`; extension comments use it too | The same template still uses `comment.recipient.name` in a recipient label. Migrate that label before claiming the whole surface is complete. | +| Dashboard | Cross-project cards display unit/task names, not another student's name; other user-bearing views were migrated in MISC-PN02 | Keep dashboard feedback metadata free of additional identity fields. | +| Search | Web `User.matches` matches `displayName`, legal first name, surname, nickname and existing identifiers | Preserve authorised search while showing only the canonical result label. | +| Notifications | `app/views/notifications_mailer/*` still has `@user.name` and `@user.first_name`; communication templates use nickname/first-name greetings | Prioritise normal notification name migration. These are ordinary correspondence, not legal-name exceptions. Keep HTML and text alternatives consistent. | +| Reports and exports | Student-list export uses the canonical helper; `app/views/portfolio/portfolio_pdf.pdf.erb` and `app/views/task/task_pdf.pdf.erb` still render legal name fields | Ask the record owner which outputs require legal names; migrate ordinary output and document approved official exceptions individually. | +| Legacy Web helper | `User.name` truncates each name and appends nickname | Do not use it in new normal display surfaces; migrate consumers by feature to avoid changing administrative output silently. | + +Reviewers should confirm the normal rule, search visibility and exception +process together. Any approval and institutional exceptions belong in the PR +review or a linked institutional decision; do not fill in assumed approvals. diff --git a/test/api/auth_test.rb b/test/api/auth_test.rb index cc3f737601..dd4ee7163c 100644 --- a/test/api/auth_test.rb +++ b/test/api/auth_test.rb @@ -9,6 +9,19 @@ def app Rails.application end + setup do + Rack::Attack.reset! + end + + def post_failed_auth(username:, ip:) + post( + '/api/auth.json', + { username: username, password: 'definitely-wrong-password' }.to_json, + 'CONTENT_TYPE' => 'application/json', + 'REMOTE_ADDR' => ip + ) + end + # --------------------------------------------------------------------------- # # --- Endpoint testing for: # ------- /api/auth.json @@ -19,6 +32,9 @@ def app # Test POST for new authentication token def test_auth_post + expected_auth = User.first + expected_auth.update!(theme_preference: 'dark') + data_to_post = { username: 'aadmin', password: 'password', @@ -27,7 +43,6 @@ def test_auth_post # Get response back for logging in with username 'aadmin' password 'password' post_json '/api/auth.json', data_to_post actual_auth = last_response_body - expected_auth = User.first # Check that response contains a user. assert actual_auth.key?('user'), 'Expect response to have a user' @@ -37,10 +52,13 @@ def test_auth_post # Check that the returned user has the required details. # These match the model object... so can compare in loops - user_keys = %w[id email first_name last_name username nickname receive_task_notifications receive_portfolio_notifications receive_feedback_notifications opt_in_to_research has_run_first_time_setup] + user_keys = %w[id email first_name last_name username nickname display_name receive_task_notifications receive_portfolio_notifications receive_feedback_notifications display_peer_progress opt_in_to_research has_run_first_time_setup theme_preference] # Check the returned user matches the expected database value assert_json_matches_model(expected_auth, response_user_data, user_keys) + assert_in_delta expected_auth.theme_preference_updated_at.to_f, + Time.iso8601(response_user_data['theme_preference_updated_at']).to_f, + 0.001 # Check other values returned assert_equal expected_auth.role.name, response_user_data['system_role'], 'Roles match' @@ -114,6 +132,40 @@ def test_fail_password_auth assert actual_auth.key? 'error' end + def test_repeated_failed_password_auth_is_rate_limited_by_ip + travel_to Time.zone.parse('2026-08-27 03:00:30 UTC') do + 6.times do |attempt| + post_failed_auth( + username: "missing-user-#{attempt}", + ip: '192.0.2.10' + ) + + assert_equal(attempt < 5 ? 401 : 429, last_response.status) + end + + assert_equal '30', last_response.headers.fetch('retry-after') + assert_equal 'Too many authentication attempts. Please try again later.', last_response_body['error'] + end + end + + def test_repeated_failed_password_auth_is_rate_limited_by_normalized_json_username + username = User.first.username + username_variants = [username, username.upcase, " #{username}", "#{username} ", " #{username.upcase} "] + + travel_to Time.zone.parse('2026-08-27 03:00:30 UTC') do + username_variants.each_with_index do |attempted_username, attempt| + post_failed_auth(username: attempted_username, ip: "198.51.100.#{attempt + 1}") + assert_equal 401, last_response.status + end + + post_failed_auth(username: username, ip: '198.51.100.6') + + assert_equal 429, last_response.status + assert_equal '30', last_response.headers.fetch('retry-after') + assert_equal 'Too many authentication attempts. Please try again later.', last_response_body['error'] + end + end + # Test auth with empty request body def test_fail_empty_request data_to_post = "" @@ -195,6 +247,7 @@ def test_auth_delete def test_refresh_token user = FactoryBot.create(:user) + user.update!(theme_preference: 'dark') token = user.generate_authentication_token!(token_type: :refresh_token) count = user.auth_tokens.count @@ -205,6 +258,10 @@ def test_refresh_token post '/api/auth/access-token', { remember: true } assert_equal 201, last_response.status + assert_equal 'dark', last_response_body.dig('user', 'theme_preference') + assert_in_delta user.theme_preference_updated_at.to_f, + Time.iso8601(last_response_body.dig('user', 'theme_preference_updated_at')).to_f, + 0.001 assert_equal count + 1, user.auth_tokens.count new_token = user.auth_tokens.last diff --git a/test/api/comments/comment_test.rb b/test/api/comments/comment_test.rb index 961b2a5e0d..f430572e04 100644 --- a/test/api/comments/comment_test.rb +++ b/test/api/comments/comment_test.rb @@ -70,6 +70,18 @@ def test_student_post_comment task_definition = unit.task_definitions.first tutor = project.tutor_for(task_definition) + user.update!( + first_name: 'Elizabeth', + last_name: 'Smith', + nickname: 'Liz' + ) + + tutor.update!( + first_name: 'Thomas', + last_name: 'Brown', + nickname: nil + ) + pre_count = TaskComment.count comment_data = { comment: 'Hello World' } @@ -89,14 +101,20 @@ def test_student_post_comment 'has_attachment' => false, 'type' => 'text', 'is_new' => false, - author: { 'id' => user.id }, - recipient: { 'id' => tutor.id } + author: { + 'id' => user.id, + 'display_name' => 'Liz Smith' + }, + recipient: { + 'id' => tutor.id, + 'display_name' => 'Thomas Brown' + } } # check each is the same assert_json_matches_model expected_response, last_response_body, %w(comment has_attachment type is_new) - assert_json_matches_model expected_response[:author], last_response_body['author'], ['id'] - assert_json_matches_model expected_response[:recipient], last_response_body['recipient'], ['id'] + assert_json_matches_model expected_response[:author], last_response_body['author'], %w[id display_name] + assert_json_matches_model expected_response[:recipient], last_response_body['recipient'], %w[id display_name] end def test_replying_to_comments @@ -522,6 +540,62 @@ def test_student_post_pdf_comment new_comment.destroy end + # Regression: an attachment (audio/image/pdf) comment must raise a + # task_comment_created notification just like a text comment. The attachment + # path used to save the comment and return without notifying, so a tutor's + # audio or pdf feedback reached the student's inbox as nothing at all. + def test_attachment_comment_notifies_the_recipient + project = FactoryBot.create(:project) + unit = project.unit + convenor = unit.main_convenor_user + student = project.student + task_definition = unit.task_definitions.first + + add_auth_header_for(user: convenor) + + comment_data = { attachment: upload_file('test_files/submissions/00_question.pdf', 'application/pdf') } + + assert_difference 'Notification.count', 1 do + post "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/comments", comment_data + end + + assert_equal 201, last_response.status, last_response_body + + notification = Notification.recent_first.first + + assert_equal student, notification.user, 'the student, not the commenter, is notified' + assert_equal 'feedback', notification.notification_type + assert_equal 'task_comment_created', notification.event + + TaskComment.last.destroy + end + + # Regression: a feedback review request must notify its recipient just like a + # text or attachment comment. The request path saved the comment and returned + # without notifying, so a student's request for a review reached the tutor as + # no email, push or in-app notification at all. + def test_feedback_review_request_notifies_the_recipient + project = FactoryBot.create(:project) + task_definition = project.unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + student = project.student + tutor = project.tutor_for(task_definition) + + assert_not_nil tutor, 'the project needs a tutor for the request to have a recipient' + + assert_difference 'Notification.count', 1 do + task.add_feedback_review_request_comment(student) + end + + notification = Notification.recent_first.first + + assert_equal tutor, notification.user, 'the tutor, not the requesting student, is notified' + assert_equal 'feedback', notification.notification_type + assert_equal 'task_comment_created', notification.event + + TaskComment.last.destroy + end + def test_comment_attachments_deleted project = Project.first user = project.student @@ -564,12 +638,120 @@ def test_post_comment_empty_attachment post "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/comments", comment_data - assert_equal 500, last_response.status + assert_equal 400, last_response.status, last_response_body assert_equal pre_count, TaskComment.count, 'No comment should be created' assert_equal 'Attachment is empty.', last_response_body['error'] end + def test_post_comment_oversized_attachment + project = Project.first + task_definition = project.unit.task_definitions.first + pre_count = TaskComment.count + + add_auth_header_for(user: project.student) + + attachment = upload_file('test_files/submissions/00_question.pdf', 'application/pdf') + File.stub :size?, 30_000_001 do + post "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/comments", { attachment: attachment } + end + + assert_equal 413, last_response.status, last_response_body + assert_equal pre_count, TaskComment.count, 'No comment should be created' + assert_equal 'Attachment exceeds the maximum attachment size of 30MB.', last_response_body['error'] + end + + # Builds a group task definition for the given group_set. + def make_group_task_definition(unit, group_set) + td = TaskDefinition.new(unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: "pr_file_01_group_task_#{group_set.id}", + description: 'group attachment access', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 1.week, + target_date: Time.zone.now - 1.day, + due_date: Time.zone.now + 1.week, + abbreviation: "PRFILE01_#{group_set.id}", + restrict_status_updates: false, + upload_requirements: [ { 'key' => 'file0', 'name' => 'Doc', 'type' => 'document' } ], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0, + group_set: group_set) + td.save! + td + end + + # Builds a single group of `members` and returns [unit, group, task_definition]. + def build_group_task(members: 2) + unit = FactoryBot.create :unit + group_set = GroupSet.create!(name: 'pr_file_01_group_set', unit: unit) + group = Group.create!(group_set: group_set, name: 'pr_file_01_group', tutorial: unit.tutorials.first) + members.times { |i| group.add_member(unit.active_projects[i]) } + group.save! + + [unit, group, make_group_task_definition(unit, group_set)] + end + + # A group member posts an image attachment. Another member of the same group must be + # able to open it. The attachment is on the author's task instance, but the whole group + # shares one group_submission, so the fetch has to look through all_comments, not the + # caller's own task's comments (which was returning ActiveRecord::RecordNotFound -> 404). + def test_group_member_can_open_another_members_attachment + _unit, group, td = build_group_task + + author = group.projects.first + reader = group.projects.second + + add_auth_header_for(user: author.student) + post "/api/projects/#{author.id}/task_def_id/#{td.id}/comments", + { attachment: upload_file('test_files/submissions/Deakin_Logo.jpeg', 'image/jpeg') } + assert_equal 201, last_response.status, last_response.body + comment_id = last_response_body['id'] + + # The other member opens the attachment through their own project. + add_auth_header_for(user: reader.student) + get "/api/projects/#{reader.id}/task_def_id/#{td.id}/comments/#{comment_id}" + assert_equal 200, last_response.status, last_response.body + + TaskComment.find(comment_id).destroy + end + + # The widened lookup must stay bounded to the caller's own group. A member of a + # different group in the same group_set, querying their own project (so the :get + # check passes), still cannot reach the first group's comment: all_comments is scoped + # by that caller's own group_submission, so the id is not found and the API returns 404. + def test_attachment_lookup_stays_within_callers_group + unit = FactoryBot.create :unit + group_set = GroupSet.create!(name: 'pr_file_01_two_groups', unit: unit) + group_a = Group.create!(group_set: group_set, name: 'pr_file_01_group_a', tutorial: unit.tutorials.first) + group_b = Group.create!(group_set: group_set, name: 'pr_file_01_group_b', tutorial: unit.tutorials.first) + group_a.add_member(unit.active_projects[0]) + group_b.add_member(unit.active_projects[1]) + td = make_group_task_definition(unit, group_set) + + author = group_a.projects.first + outsider = group_b.projects.first + + add_auth_header_for(user: author.student) + post "/api/projects/#{author.id}/task_def_id/#{td.id}/comments", + { attachment: upload_file('test_files/submissions/Deakin_Logo.jpeg', 'image/jpeg') } + assert_equal 201, last_response.status, last_response.body + comment_id = last_response_body['id'] + + # The other group's member posts on their own group task, so their task and + # group_submission exist, then tries to open group A's attachment. + add_auth_header_for(user: outsider.student) + post_json "/api/projects/#{outsider.id}/task_def_id/#{td.id}/comments", comment: 'group b note' + assert_equal 201, last_response.status, last_response.body + + get "/api/projects/#{outsider.id}/task_def_id/#{td.id}/comments/#{comment_id}" + assert_equal 404, last_response.status, last_response.body + + TaskComment.find(comment_id).destroy + end + def test_read_receipts_for_task_status_comments project = Project.first user = project.student @@ -704,4 +886,115 @@ def test_discussed_in_class_task_comments_dont_show_in_inbox td.destroy! end + + # Marking a comment as unread must delete the caller's read receipt and succeed. + # remove_comment_read_entry used to call delete_all with a conditions hash, which raises + # ArgumentError on Rails 8 and turned every mark-as-unread into a 500. + def test_mark_comment_as_unread_removes_the_read_receipt + project = FactoryBot.create(:project) + unit = project.unit + user = project.student + convenor = unit.main_convenor_user + task_definition = unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + + comment = task.add_text_comment(convenor, 'Please look at this') + comment.mark_as_read(user) + assert comment.read_by?(user), 'Comment should be read before it is marked unread' + assert_equal 1, CommentsReadReceipts.where(user: user, task_comment: comment).count + + add_auth_header_for user: user + post "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/comments/#{comment.id}" + + assert_equal 201, last_response.status, last_response_body + assert_equal 0, CommentsReadReceipts.where(user: user, task_comment: comment).count, 'Read receipt should be gone' + assert_not comment.reload.read_by?(user), 'Comment should be unread after the request' + end + + def test_group_member_can_mark_shared_comment_unread_without_affecting_other_receipts + fixture = grouped_comment_fixture + comment = fixture[:comment] + author = fixture[:first_project].student + caller = fixture[:second_project].student + + comment.mark_as_read(caller) + assert comment.read_by?(author), "the comment author's receipt should exist" + assert comment.read_by?(caller), "the other group member's receipt should exist" + + add_auth_header_for user: caller + post "/api/projects/#{fixture[:second_project].id}/task_def_id/#{fixture[:task_definition].id}/comments/#{comment.id}" + + assert_equal 201, last_response.status, last_response_body + assert_not comment.reload.read_by?(caller), "only the caller's receipt should be removed" + assert comment.read_by?(author), "another group member's receipt must remain" + end + + def test_member_of_another_group_cannot_mark_comment_unread + fixture = grouped_comment_fixture + comment = fixture[:comment] + outsider = fixture[:other_project].student + + # Give the other group its own submission so all_comments is explicitly + # scoped to that group submission rather than the individual task. + fixture[:other_project] + .task_for_task_definition(fixture[:task_definition]) + .ensured_group_submission + + comment.mark_as_read(outsider) + assert comment.read_by?(outsider) + + add_auth_header_for user: outsider + post "/api/projects/#{fixture[:other_project].id}/task_def_id/#{fixture[:task_definition].id}/comments/#{comment.id}" + + assert_equal 404, last_response.status, last_response_body + assert comment.reload.read_by?(outsider), 'a rejected request must not change receipts' + end + + # A user with no submission rights on the project cannot mark its comments unread. + def test_mark_comment_as_unread_rejects_an_unauthorised_user + project = FactoryBot.create(:project) + unit = project.unit + convenor = unit.main_convenor_user + task_definition = unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + comment = task.add_text_comment(convenor, 'Private thread') + + outsider = FactoryBot.create(:project).student + + add_auth_header_for user: outsider + post "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/comments/#{comment.id}" + + assert_equal 403, last_response.status, last_response_body + end + + private + + def grouped_comment_fixture + unit = FactoryBot.create(:unit, student_count: 3, task_count: 0) + first_project, second_project, other_project = unit.active_projects.first(3) + group_set = FactoryBot.create(:group_set, unit: unit) + shared_group = FactoryBot.create(:group, group_set: group_set, tutorial: unit.tutorials.first) + other_group = FactoryBot.create(:group, group_set: group_set, tutorial: unit.tutorials.first) + + shared_group.add_member(first_project) + shared_group.add_member(second_project) + other_group.add_member(other_project) + + task_definition = FactoryBot.create( + :task_definition, + unit: unit, + group_set: group_set, + outcome_count: 0 + ) + task = first_project.task_for_task_definition(task_definition) + comment = task.add_text_comment(first_project.student, 'Shared group feedback') + + { + first_project: first_project, + second_project: second_project, + other_project: other_project, + task_definition: task_definition, + comment: comment + } + end end diff --git a/test/api/discussion_comment_api_test.rb b/test/api/discussion_comment_api_test.rb new file mode 100644 index 0000000000..4a5028f8db --- /dev/null +++ b/test/api/discussion_comment_api_test.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'test_helper' + +class DiscussionCommentApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + include TestHelpers::TestFileHelper + + def app + Rails.application + end + + setup do + @project = FactoryBot.create(:project) + @unit = @project.unit + @task_definition = @unit.task_definitions.first + @task = @project.task_for_task_definition(@task_definition) + @student = @project.student + @tutor = FactoryBot.create(:user, :tutor) + @unit.employ_staff(@tutor, Role.tutor) + end + + def test_create_discussion_comment_rejects_empty_attachment_with_bad_request + add_auth_header_for(user: @tutor) + comment_count = DiscussionComment.count + + post discussion_comments_endpoint, { + attachments: [upload_file('test_files/submissions/boo.png', 'audio/wav')] + } + + assert_equal 400, last_response.status, last_response_body + assert_equal 'Attachment is empty.', last_response_body['error'] + assert_equal comment_count, DiscussionComment.count + end + + def test_create_discussion_comment_rejects_oversized_attachment_with_payload_too_large + add_auth_header_for(user: @tutor) + comment_count = DiscussionComment.count + attachment = upload_file('test_files/submissions/00_question.pdf', 'audio/wav') + + File.stub :size?, 30_000_001 do + post discussion_comments_endpoint, { attachments: [attachment] } + end + + assert_equal 413, last_response.status, last_response_body + assert_equal 'Attachment exceeds the maximum attachment size of 30MB.', last_response_body['error'] + assert_equal comment_count, DiscussionComment.count + end + + def test_discussion_reply_rejects_empty_attachment_with_bad_request + discussion = create_discussion_comment + add_auth_header_for(user: @student) + + post discussion_reply_endpoint(discussion), { + attachment: upload_file('test_files/submissions/boo.png', 'audio/wav') + } + + assert_equal 400, last_response.status, last_response_body + assert_equal 'Attachment is empty.', last_response_body['error'] + assert_nil discussion.reload.time_discussion_completed + end + + def test_discussion_reply_rejects_oversized_attachment_with_payload_too_large + discussion = create_discussion_comment + add_auth_header_for(user: @student) + attachment = upload_file('test_files/submissions/00_question.pdf', 'audio/wav') + + File.stub :size?, 30_000_001 do + post discussion_reply_endpoint(discussion), { attachment: attachment } + end + + assert_equal 413, last_response.status, last_response_body + assert_equal 'Attachment exceeds the maximum attachment size of 30MB.', last_response_body['error'] + assert_nil discussion.reload.time_discussion_completed + end + + private + + def discussion_comments_endpoint + "/api/projects/#{@project.id}/task_def_id/#{@task_definition.id}/discussion_comments" + end + + def discussion_reply_endpoint(discussion) + "/api/projects/#{@project.id}/task_def_id/#{@task_definition.id}/comments/#{discussion.id}/discussion_comment/reply" + end + + def create_discussion_comment + DiscussionComment.create!( + task: @task, + user: @tutor, + recipient: @student, + content_type: 'discussion', + number_of_prompts: 1 + ) + end +end diff --git a/test/api/task_grade_authorisation_test.rb b/test/api/task_grade_authorisation_test.rb new file mode 100644 index 0000000000..e8a7a03a78 --- /dev/null +++ b/test/api/task_grade_authorisation_test.rb @@ -0,0 +1,144 @@ +require 'test_helper' + +# +# Tests that writing the grade of a task through the task update endpoint +# requires the assessment permission, and that the ordinary student +# submission path through the same endpoint is unaffected. +# +class TaskGradeAuthorisationTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + # Creates a unit with one student and a single graded task definition that + # needs no uploaded documents. + def create_unit_with_graded_task + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = TaskDefinition.create!({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Graded task', + description: 'Graded task', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + abbreviation: 'GradedTask', + restrict_status_updates: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: true, + max_quality_pts: 0 + }) + + [unit, td] + end + + # The unit factory only ever employs convenors, so a tutor has to be added + # explicitly for the tutor case to be a tutor rather than a second convenor. + def employ_tutor(unit) + tutor = FactoryBot.create(:user, :tutor) + unit.employ_staff(tutor, Role.tutor) + tutor + end + + def test_student_cannot_set_grade_on_own_task + unit, td = create_unit_with_graded_task + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + add_auth_header_for(user: project.student) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { grade: 3 } + + assert_equal 403, last_response.status, last_response_body + assert_equal 'You are not permitted to assess this task', last_response_body['error'] + + task.reload + assert_nil task.grade + + unit.destroy + end + + def test_tutor_can_set_grade + unit, td = create_unit_with_graded_task + project = unit.active_projects.first + task = project.task_for_task_definition(td) + tutor = employ_tutor(unit) + + assert_equal Role.tutor, tutor.role + assert_equal :tutor, project.user_role(tutor) + + add_auth_header_for(user: tutor) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { grade: 3 } + + assert_equal 200, last_response.status, last_response_body + + task.reload + assert_equal 3, task.grade + + unit.destroy + end + + def test_student_submission_without_grade_still_succeeds + unit, td = create_unit_with_graded_task + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + add_auth_header_for(user: project.student) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'ready_for_feedback' } + + assert_equal 200, last_response.status, last_response_body + + task.reload + assert_equal TaskStatus.ready_for_feedback, task.task_status + assert_nil task.grade + + unit.destroy + end + + # A refused request must not have moved the status on its way to the 403. + def test_student_grade_with_trigger_changes_nothing + unit, td = create_unit_with_graded_task + project = unit.active_projects.first + task = project.task_for_task_definition(td) + status_before = task.task_status + + add_auth_header_for(user: project.student) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'ready_for_feedback', grade: 3 } + + assert_equal 403, last_response.status, last_response_body + assert_equal 'You are not permitted to assess this task', last_response_body['error'] + + task.reload + assert_nil task.grade + assert_equal status_before, task.task_status + assert_equal 0, task.task_submissions.count + + unit.destroy + end + + def test_convenor_can_set_grade + unit, td = create_unit_with_graded_task + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + add_auth_header_for(user: unit.main_convenor_user) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { grade: 2 } + + assert_equal 200, last_response.status, last_response_body + + task.reload + assert_equal 2, task.grade + + unit.destroy + end +end diff --git a/test/api/tasks_api_test.rb b/test/api/tasks_api_test.rb index 016f789e7f..fa4a451f7d 100644 --- a/test/api/tasks_api_test.rb +++ b/test/api/tasks_api_test.rb @@ -85,6 +85,11 @@ def test_get_task_submission_details_creates_session_and_activity # Make the request get "/api/projects/#{project.id}/task_def_id/#{td.id}/submission_details" + assert_equal 'not_submitted', last_response_body['processing_state'] + assert_equal false, last_response_body['pdf_ready'] + assert_equal false, last_response_body['submission_files_ready'] + assert_equal false, last_response_body['retryable'] + # Check if counts increased session_count_after = MarkingSession.count activity_count_after = SessionActivity.count @@ -832,14 +837,231 @@ def test_requires_discussion_blocks_complete_until_discussed_comment_added assert_equal TaskStatus.complete, task.task_status end + # discussed:true marks a task as discussed in class; discussed:false must unmark + # it by removing every marker, including legacy duplicates separated by an + # ordinary feedback comment (DOM-07). + def test_discussed_false_removes_all_discussed_comments + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = TaskDefinition.create!({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Discussed toggle task', + description: 'Task used to toggle the discussed mark', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + abbreviation: 'DiscussToggleTask', + restrict_status_updates: false, + requires_discussion: true, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + + project = unit.active_projects.first + task = project.task_for_task_definition(td) + tutor = unit.tutors.first + + add_auth_header_for(user: tutor) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { discussed: true } + assert_equal 200, last_response.status + task.reload + assert task.has_discussed_in_class_comment?, 'discussed:true should mark the task as discussed' + assert_equal 1, task.comments.where(content_type: 'discussed_in_class').count + + task.add_text_comment(tutor, 'Feedback between legacy discussed markers') + + # add_discussed_comment now treats the marker as a boolean and will not + # create another one just because feedback was added after it. + task.add_discussed_comment(tutor) + assert_equal 1, task.comments.where(content_type: 'discussed_in_class').count + + # Reproduce legacy data written before duplicate prevention was added. + duplicate = TaskDiscussedComment.create!( + task: task, + user: tutor, + recipient: project.student, + comment: 'Discussed in class' + ) + duplicate_receipt_ids = duplicate.comments_read_receipts.ids + assert_equal 2, task.comments.where(content_type: 'discussed_in_class').count + assert_not_empty duplicate_receipt_ids + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { discussed: false } + assert_equal 200, last_response.status + task.reload + assert_not task.has_discussed_in_class_comment?, 'discussed:false should unmark the task, not add another comment' + assert_equal 0, task.comments.where(content_type: 'discussed_in_class').count + assert_empty CommentsReadReceipts.where(id: duplicate_receipt_ids), 'destroy callbacks must remove marker read receipts' + + unit.destroy + end + + # A completed task in a unit that requires discussion cannot have its discussed + # mark removed, since that would leave it complete without the evidence the + # model requires (DOM-07). + def test_discussed_false_rejected_when_the_task_is_complete + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = TaskDefinition.create!({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Discussed complete guard task', + description: 'Task used to guard unmarking after complete', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + abbreviation: 'DiscussGuardTask', + restrict_status_updates: false, + requires_discussion: true, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + + project = unit.active_projects.first + task = project.task_for_task_definition(td) + tutor = unit.tutors.first + + add_auth_header_for(user: tutor) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { discussed: true } + assert_equal 200, last_response.status + task.add_text_comment(tutor, 'Manual tutor feedback') + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'complete' } + assert_equal 200, last_response.status + task.reload + assert_equal TaskStatus.complete, task.task_status + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { discussed: false } + assert_equal 403, last_response.status + task.reload + assert task.has_discussed_in_class_comment?, 'the discussed comment must survive a refused unmark' + assert_equal TaskStatus.complete, task.task_status + + unit.destroy + end + + # A helper for the refused-transition tests below. An ordinary task definition, + # nothing about it restricted, so the only reason a transition can be refused is + # the one the test is asking about. + def ordinary_task_definition_for(unit, restrict: false) + TaskDefinition.create!({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: "Refusal reporting task #{restrict}", + description: 'Task used to check refused transitions are reported', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + abbreviation: "RefuseTask#{restrict ? 'R' : 'O'}", + restrict_status_updates: restrict, + requires_discussion: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + end + + # A student asking for a staff status is refused inside trigger_transition, which + # returns nil and adds no error. That used to reach the 200 at the end of the + # handler, so the client showed the change as accepted. + def test_refused_transition_to_a_staff_status_returns_forbidden + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = ordinary_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + status_before = task.task_status + + add_auth_header_for(user: project.student) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'complete' } + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This status change is not allowed for this task.', last_response_body['error'] + + task.reload + assert_equal status_before, task.task_status + end + + # An unrecognised trigger string falls through the case statement and is refused + # the same silent way, whoever sends it. + def test_unrecognised_trigger_returns_forbidden + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = ordinary_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + status_before = task.task_status + + add_auth_header_for(user: unit.tutors.first) + + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'competed' } + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This status change is not allowed for this task.', last_response_body['error'] + + task.reload + assert_equal status_before, task.task_status + end + + # The regression check. This change makes a permissive endpoint strict, so the + # failure mode is that ordinary marking stops working. + def test_allowed_transitions_still_return_success + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = ordinary_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + add_auth_header_for(user: project.student) + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'working_on_it' } + assert_equal 200, last_response.status, last_response.body + task.reload + assert_equal TaskStatus.working_on_it, task.task_status + + add_auth_header_for(user: unit.tutors.first) + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'discuss' } + assert_equal 200, last_response.status, last_response.body + task.reload + assert_equal TaskStatus.discuss, task.task_status + end + + # The restricted message is the one sentence in this endpoint that tells a + # student something they can act on, so it has to survive ahead of the generic + # one. Nothing in the test tree protected it before. + def test_restricted_task_keeps_its_own_refusal_message + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + td = ordinary_task_definition_for(unit, restrict: true) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + # Put the task at a staff assigned status first, which is the condition the + # restricted guard actually tests. + add_auth_header_for(user: unit.tutors.first) + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'discuss' } + assert_equal 200, last_response.status, last_response.body + + add_auth_header_for(user: project.student) + put "/api/projects/#{project.id}/task_def_id/#{td.id}", { trigger: 'working_on_it' } + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This task can only be updated by your tutor.', last_response_body['error'] + + task.reload + assert_equal TaskStatus.discuss, task.task_status + end + def test_require_comment_for_feedback_submission_assess_in_portfolio unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) td1 = unit.task_definitions.first project = unit.active_projects.first - task = project.task_for_task_definition(td1) - - td1.update( + td1.update!( upload_requirements: [{ "key" => 'file0', "name" => 'Shape Class', "type" => 'code' }], target_grade: 0, # Pass start_date: Time.zone.now - 2.weeks, @@ -847,6 +1069,8 @@ def test_require_comment_for_feedback_submission_assess_in_portfolio assess_in_portfolio_only: false ) + task = project.task_for_task_definition(td1) + add_auth_header_for(user: project.user) # Use a direct submit here so the test can focus on the comment requirement. @@ -889,6 +1113,100 @@ def test_require_comment_for_feedback_submission_assess_in_portfolio assert_equal comment, text_comment.comment end + # A task definition with one upload requirement, used by the finalised-task + # upload tests below. + def uploadable_task_definition_for(unit) + td = unit.task_definitions.first + td.update!( + upload_requirements: [{ "key" => 'file0', "name" => 'Shape Class', "type" => 'code' }], + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + assess_in_portfolio_only: false, + restrict_status_updates: false + ) + td + end + + # A signed off task used to keep accepting uploads. The upload rewrote + # submission_date and file_uploaded_at and deleted the assessed pdf, and only + # the status transition was skipped, so the damage was silent. + def test_student_cannot_upload_to_a_complete_task + unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) + td = uploadable_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + task.update!(task_status: TaskStatus.complete, submission_date: Time.zone.now - 1.day) + submission_date_before = task.reload.submission_date + + add_auth_header_for(user: project.user) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", + with_file('test_files/submissions/program.cs', 'application/json', { trigger: 'ready_for_feedback' }) + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This task is closed for new submissions.', last_response_body['error'] + + task.reload + assert_equal TaskStatus.complete, task.task_status + assert_equal submission_date_before.to_i, task.submission_date.to_i + end + + # feedback_exceeded is the state students are otherwise barred from leaving, so + # it is the one where a silent upload is most misleading. + def test_student_cannot_upload_to_a_feedback_exceeded_task + unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) + td = uploadable_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + task.update!(task_status: TaskStatus.feedback_exceeded, submission_date: Time.zone.now - 1.day) + submission_date_before = task.reload.submission_date + + add_auth_header_for(user: project.user) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", + with_file('test_files/submissions/program.cs', 'application/json', { trigger: 'ready_for_feedback' }) + + assert_equal 403, last_response.status, last_response.body + + task.reload + assert_equal TaskStatus.feedback_exceeded, task.task_status + assert_equal submission_date_before.to_i, task.submission_date.to_i + end + + # Staff go through on purpose. A tutor uploads on a student's behalf when a file + # is corrupt or was submitted against the wrong task. + def test_staff_can_still_upload_to_a_complete_task + unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) + td = uploadable_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + task.update!(task_status: TaskStatus.complete) + + add_auth_header_for(user: unit.main_convenor_user) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", + with_file('test_files/submissions/program.cs', 'application/json', { trigger: 'ready_for_feedback' }) + + assert_equal 201, last_response.status, last_response.body + end + + # The regression check. Ordinary resubmission is untouched. + def test_student_can_still_upload_to_an_open_task + unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) + td = uploadable_task_definition_for(unit) + project = unit.active_projects.first + task = project.task_for_task_definition(td) + + task.update!(task_status: TaskStatus.ready_for_feedback) + + add_auth_header_for(user: project.user) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", + with_file('test_files/submissions/program.cs', 'application/json', { trigger: 'ready_for_feedback' }) + + assert_equal 201, last_response.status, last_response.body + end + def test_resubmission_doesnt_change_submission_date Sidekiq::Testing.inline! do unit = FactoryBot.create( diff --git a/test/api/units/similarity_scan_test.rb b/test/api/units/similarity_scan_test.rb new file mode 100644 index 0000000000..491db8dc93 --- /dev/null +++ b/test/api/units/similarity_scan_test.rb @@ -0,0 +1,52 @@ +require 'test_helper' + +# Covers POST /units/:id/similarity/scan, the on-demand plagiarism rescan. +class UnitsSimilarityScanApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + # A tutor is not one of the roles granted :run_similarity_scan, so the endpoint + # must refuse before it queues anything. + def test_tutor_cannot_run_similarity_scan + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + tutor = FactoryBot.create(:user, :tutor) + unit.employ_staff(tutor, Role.tutor) + + add_auth_header_for(user: tutor) + post "/api/units/#{unit.id}/similarity/scan" + + assert_equal 403, last_response.status, last_response_body + end + + # A scan recorded in the last 30 minutes puts the unit inside the cooldown, so a + # convenor's request is rate limited rather than queuing a second scan. + def test_similarity_scan_is_rate_limited_within_the_cooldown + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + unit.update!(last_plagarism_scan: Time.zone.now) + + add_auth_header_for(user: unit.main_convenor_user) + post "/api/units/#{unit.id}/similarity/scan" + + assert_equal 429, last_response.status, last_response_body + end + + # sidekiq-unique-jobs returns nil when its :reject conflict strategy refuses a + # duplicate. That is distinct from the completed-scan cooldown above: the first + # job may still be queued or running and therefore has not stamped the unit yet. + def test_similarity_scan_returns_conflict_when_duplicate_enqueue_is_rejected + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + add_auth_header_for(user: unit.main_convenor_user) + CheckUnitSimilarityJob.stub(:perform_async, nil) do + post "/api/units/#{unit.id}/similarity/scan" + end + + assert_equal 409, last_response.status, last_response_body + assert_equal 'A similarity scan is already queued or running for this unit.', last_response_body['error'] + end +end diff --git a/test/api/users_test.rb b/test/api/users_test.rb index 05b21395c5..0cd53ff217 100644 --- a/test/api/users_test.rb +++ b/test/api/users_test.rb @@ -11,8 +11,11 @@ def app def assert_users_model_response(response_data, user_model, keys = nil) if keys.nil? - keys = %w[id student_id email first_name last_name username nickname receive_task_notifications - receive_portfolio_notifications receive_feedback_notifications opt_in_to_research has_run_first_time_setup] + keys = %w[id student_id email first_name last_name username nickname display_name receive_task_notifications + receive_portfolio_notifications receive_feedback_notifications display_peer_progress + opt_in_to_research has_run_first_time_setup] + assert_not response_data.key?('theme_preference') + assert_not response_data.key?('theme_preference_updated_at') end assert_json_matches_model(user_model, response_data, keys) @@ -50,7 +53,7 @@ def test_get_users assert_equal expected_data.count, last_response_body.count # What are the keys we expect in the data that match the model - so we can check these - response_keys = %w[first_name last_name email student_id nickname receive_task_notifications receive_portfolio_notifications receive_feedback_notifications opt_in_to_research has_run_first_time_setup] + response_keys = %w[first_name last_name email student_id nickname display_name receive_task_notifications receive_portfolio_notifications receive_feedback_notifications display_peer_progress opt_in_to_research has_run_first_time_setup] # Loop through all of the responses last_response_body.each do | data | @@ -58,6 +61,8 @@ def test_get_users user = User.find(data['id']) # Match json with object assert_json_matches_model(user, data, response_keys) + assert_not data.key?('theme_preference') + assert_not data.key?('theme_preference_updated_at') end end @@ -76,8 +81,10 @@ def test_get_a_users_details assert_equal 200, last_response.status # Check the returned details match as expected - response_keys = %w(first_name last_name email student_id nickname receive_task_notifications receive_portfolio_notifications receive_feedback_notifications opt_in_to_research has_run_first_time_setup) + response_keys = %w[first_name last_name email student_id nickname display_name receive_task_notifications receive_portfolio_notifications receive_feedback_notifications display_peer_progress opt_in_to_research has_run_first_time_setup] assert_json_matches_model(expected_user, returned_user, response_keys) + assert_not returned_user.key?('theme_preference') + assert_not returned_user.key?('theme_preference_updated_at') end def test_get_convenors @@ -87,6 +94,10 @@ def test_get_convenors get '/api/users/convenors' assert_equal 200, last_response.status + last_response_body.each do |user| + assert_not user.key?('theme_preference') + assert_not user.key?('theme_preference_updated_at') + end end def test_get_tutors @@ -96,6 +107,10 @@ def test_get_tutors get '/api/users/tutors' assert_equal 200, last_response.status + last_response_body.each do |user| + assert_not user.key?('theme_preference') + assert_not user.key?('theme_preference_updated_at') + end end def test_get_no_token @@ -134,6 +149,7 @@ def test_post_create_user assert_equal pre_count + 1, User.all.length assert_users_model_response last_response_body, User.last + assert User.last.display_peer_progress? assert_equal 201, last_response.status end @@ -335,6 +351,139 @@ def test_put_update_user_existing_email assert_equal 400, last_response.status end + def test_put_update_peer_progress_display_preference + user = User.second + add_auth_header_for(user: User.first) + + put_json "/api/users/#{user.id}", { + user: { display_peer_progress: false } + } + + assert_equal 200, last_response.status + assert_equal false, last_response_body['display_peer_progress'] + assert_not user.reload.display_peer_progress? + + put_json "/api/users/#{user.id}", { + user: { display_peer_progress: true } + } + + assert_equal 200, last_response.status + assert_equal true, last_response_body['display_peer_progress'] + assert user.reload.display_peer_progress? + end + + def test_theme_preference_is_nullable_until_the_user_chooses + user = User.first + user.update!(theme_preference: nil) + add_auth_header_for(user: user) + + get "/api/users/#{user.id}" + + assert_equal 200, last_response.status + assert last_response_body.key?('theme_preference') + assert last_response_body.key?('theme_preference_updated_at') + assert_nil last_response_body['theme_preference'] + assert_nil last_response_body['theme_preference_updated_at'] + end + + def test_put_update_theme_preference_stamps_and_serializes_its_timestamp + user = User.first + user.update!(theme_preference: nil) + add_auth_header_for(user: user) + chosen_at = Time.zone.parse('2026-08-30 10:00:00 UTC') + + travel_to chosen_at do + put_json "/api/users/#{user.id}", { + user: { theme_preference: 'dark' } + } + end + + assert_equal 200, last_response.status + assert_equal 'dark', last_response_body['theme_preference'] + assert_equal chosen_at, Time.iso8601(last_response_body['theme_preference_updated_at']) + assert_equal chosen_at, user.reload.theme_preference_updated_at + end + + def test_put_same_theme_preference_refreshes_the_sync_timestamp + user = User.first + first_choice_at = Time.zone.parse('2026-08-30 10:00:00 UTC') + travel_to first_choice_at do + user.update!(theme_preference: 'dark') + end + add_auth_header_for(user: user) + + synchronization_at = first_choice_at + 2.hours + travel_to synchronization_at do + put_json "/api/users/#{user.id}", { + user: { theme_preference: 'dark' } + } + end + + assert_equal 200, last_response.status + assert_equal 'dark', last_response_body['theme_preference'] + assert_equal synchronization_at, Time.iso8601(last_response_body['theme_preference_updated_at']) + assert_equal synchronization_at, user.reload.theme_preference_updated_at + end + + def test_put_clear_theme_preference_restores_the_never_chosen_state + user = User.first + user.update!(theme_preference: 'dark') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { theme_preference: nil } + } + + assert_equal 200, last_response.status + assert last_response_body.key?('theme_preference') + assert last_response_body.key?('theme_preference_updated_at') + assert_nil last_response_body['theme_preference'] + assert_nil last_response_body['theme_preference_updated_at'] + assert_nil user.reload.theme_preference + assert_nil user.theme_preference_updated_at + end + + def test_non_self_update_ignores_theme_preference_and_omits_it_from_response + current_user = User.first + other_user = User.second + original_choice_at = Time.zone.parse('2026-08-30 10:00:00 UTC') + travel_to original_choice_at do + other_user.update!(theme_preference: 'dark') + end + add_auth_header_for(user: current_user) + + put_json "/api/users/#{other_user.id}", { + user: { + nickname: 'Updated by administrator', + theme_preference: 'light' + } + } + + assert_equal 200, last_response.status + assert_equal 'Updated by administrator', other_user.reload.nickname + assert_equal 'dark', other_user.theme_preference + assert_equal original_choice_at, other_user.theme_preference_updated_at + assert_not last_response_body.key?('theme_preference') + assert_not last_response_body.key?('theme_preference_updated_at') + end + + def test_put_invalid_theme_preference_keeps_the_existing_choice_and_timestamp + user = User.first + chosen_at = Time.zone.parse('2026-08-30 10:00:00 UTC') + travel_to chosen_at do + user.update!(theme_preference: 'dark') + end + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { theme_preference: 'sepia' } + } + + assert_equal 400, last_response.status + assert_equal 'dark', user.reload.theme_preference + assert_equal chosen_at, user.theme_preference_updated_at + end + def test_put_update_user_invalid_email user = User.second @@ -394,4 +543,170 @@ def test_put_update_user_empty_token test_put_update_user_custom_token '' end + # Regression for the promote/demote branch: changing an Auditor's role used to + # hit `action - :promote_user` (a minus, not an assignment), which raised a + # NameError and returned 500 for every auditor role change. Promoting an + # auditor to tutor must now succeed. + def test_put_promote_auditor_to_tutor + admin = FactoryBot.create(:user, :admin) + auditor = FactoryBot.create(:user, :auditor) + + add_auth_header_for(user: admin) + put_json "/api/users/#{auditor.id}", { user: { system_role: 'Tutor' } } + + assert_equal 200, last_response.status, last_response_body + assert_equal Role.tutor.id, auditor.reload.role_id + end + + def test_self_cannot_forge_a_student_id + user = FactoryBot.create(:user, student_id: 'original-id') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { student_id: 'forged-id', nickname: 'Still editable' } + } + + assert_equal 422, last_response.status + assert_equal 'original-id', user.reload.student_id + end + + def test_sso_controlled_email_rejects_self_and_admin_forgery + with_auth_method(:saml) do + user = FactoryBot.create(:user, email: 'institutional@example.edu') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { user: { email: 'forged@example.org' } } + assert_equal 422, last_response.status + assert_equal 'institutional@example.edu', user.reload.email + + put_json "/api/users/#{user.id}", { user: { email: 'INSTITUTIONAL@example.edu' } } + assert_equal 422, last_response.status + assert_equal 'institutional@example.edu', user.reload.email + + admin = FactoryBot.create(:user, :admin) + add_auth_header_for(user: admin) + put_json "/api/users/#{user.id}", { user: { email: 'admin-forged@example.org' } } + assert_equal 422, last_response.status + assert_equal 'institutional@example.edu', user.reload.email + end + end + + def test_sso_user_can_still_save_preferred_name_and_preferences + with_auth_method(:saml) do + user = FactoryBot.create(:user, email: 'institutional@example.edu') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { + email: user.email, + student_id: user.student_id, + nickname: 'Preferred', + receive_feedback_notifications: false + } + } + + assert_equal 200, last_response.status + assert_equal 'Preferred', user.reload.nickname + assert_not user.receive_feedback_notifications? + assert_equal false, last_response_body['email_editable'] + assert_equal true, last_response_body['institutional_identity_managed'] + end + end + + def test_sso_staff_identity_is_read_only_while_genuine_settings_still_save + with_auth_method(:saml) do + staff = FactoryBot.create(:user, :convenor, email: 'staff-institutional@example.edu') + add_auth_header_for(user: staff) + + put_json "/api/users/#{staff.id}", { + user: { + email: staff.email, + nickname: 'Staff preferred name', + receive_feedback_notifications: false + } + } + + assert_equal 200, last_response.status + assert_equal 'Staff preferred name', staff.reload.nickname + assert_not staff.receive_feedback_notifications? + + put_json "/api/users/#{staff.id}", { + user: { email: 'staff-forged@example.org' } + } + + assert_equal 422, last_response.status + assert_equal 'staff-institutional@example.edu', staff.reload.email + end + end + + def test_local_accounts_retain_email_and_admin_student_id_maintenance + with_auth_method(:database) do + local_user = FactoryBot.create(:user, email: 'local@example.test') + add_auth_header_for(user: local_user) + put_json "/api/users/#{local_user.id}", { user: { email: 'changed@example.test' } } + + assert_equal 200, last_response.status + assert_equal 'changed@example.test', local_user.reload.email + assert_equal true, last_response_body['email_editable'] + assert_equal false, last_response_body['institutional_identity_managed'] + + admin = FactoryBot.create(:user, :admin) + add_auth_header_for(user: admin) + put_json "/api/users/#{local_user.id}", { user: { student_id: 'admin-maintained' } } + + assert_equal 200, last_response.status + assert_equal 'admin-maintained', local_user.reload.student_id + end + end + + # MISC-PN03: a blank preferred name falls back to the legal name + def test_preferred_name_can_be_blank_and_falls_back_safely + user = FactoryBot.create(:user, first_name: 'Shaashwat', last_name: 'Sharma', nickname: 'Preferred') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { + email: user.email, + student_id: user.student_id, + nickname: '' + } + } + + assert_equal 200, last_response.status + assert_equal '', user.reload.nickname + assert_equal 'Shaashwat', user.first_name + assert_equal 'Sharma', user.last_name + assert_equal 'Shaashwat Sharma', last_response_body['display_name'] + end + + # MISC-PN03: two users sharing a preferred name keep their own legal identity + def test_duplicate_preferred_names_do_not_change_legal_identity + first_user = FactoryBot.create(:user, first_name: 'Shaashwat', last_name: 'Sharma', nickname: 'Sam') + second_user = FactoryBot.create(:user, first_name: 'Different', last_name: 'Student', nickname: 'Sam') + add_auth_header_for(user: FactoryBot.create(:user, :admin)) + + get "/api/users/#{first_user.id}" + assert_equal 200, last_response.status + first_body = last_response_body + + get "/api/users/#{second_user.id}" + assert_equal 200, last_response.status + second_body = last_response_body + + assert_equal ['Sam', 'Shaashwat', 'Sharma', 'Sam Sharma'], + first_body.values_at('nickname', 'first_name', 'last_name', 'display_name') + assert_equal ['Sam', 'Different', 'Student', 'Sam Student'], + second_body.values_at('nickname', 'first_name', 'last_name', 'display_name') + end + + private + + def with_auth_method(method) + previous = Doubtfire::Application.config.auth_method + Doubtfire::Application.config.auth_method = method + yield + ensure + Doubtfire::Application.config.auth_method = previous + end + end diff --git a/test/models/task_test.rb b/test/models/task_test.rb index 3597d8a86d..e432bd9ed1 100644 --- a/test/models/task_test.rb +++ b/test/models/task_test.rb @@ -344,18 +344,21 @@ def test_image_upload project = unit.active_projects.first + task = project.task_for_task_definition(td) + clear_submission(task) + add_auth_header_for user: unit.main_convenor_user post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post - assert_equal 201, last_response.status + assert_equal 201, last_response.status, last_response_body - task = project.task_for_task_definition(td) task.move_files_to_in_process(FileHelper.student_work_dir(:new)) assert File.exist? "#{Doubtfire::Application.config.student_work_dir}/in_process/#{task.id}/000-image.jpg" - - td.destroy + ensure + clear_submission(task) if task + td&.destroy end def test_pdf_creation_with_jpg @@ -647,155 +650,7 @@ def test_ipynb_to_pdf unit.destroy! end - def test_code_submission_with_long_lines - unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) - td = TaskDefinition.new({ - unit_id: unit.id, - tutorial_stream: unit.tutorial_streams.first, - name: 'Task with super ling lines in code submission', - description: 'Code task', - weighting: 4, - target_grade: 0, - start_date: unit.start_date + 1.week, - target_date: unit.start_date + 2.weeks, - abbreviation: 'Long', - restrict_status_updates: false, - upload_requirements: [ { "key" => 'file0', "name" => 'long.py', "type" => 'code' } ], - plagiarism_warn_pct: 0.8, - is_graded: false, - max_quality_pts: 0 - }) - td.save! - - data_to_post = { - trigger: 'ready_for_feedback' - } - - data_to_post = with_file('test_files/submissions/long.py', 'application/json', data_to_post) - - project = unit.active_projects.first - - add_auth_header_for user: unit.main_convenor_user - - post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post - - assert_equal 201, last_response.status, last_response_body - - # test submission generation - task = project.task_for_task_definition(td) - assert task.convert_submission_to_pdf(log_to_stdout: true) - path = task.zip_file_path_for_done_task - assert path - assert File.exist? path - assert File.exist? task.final_pdf_path - - # ensure the notice is included when rendered files are truncated - reader = PDF::Reader.new(task.final_pdf_path) - assert reader.pages[1].text.include? "This file has additional line breaks applied" - - # submit a normal file and ensure the notice is not included in the PDF - data_to_post = { - trigger: 'ready_for_feedback' - } - - data_to_post = with_file('test_files/submissions/normal.py', 'application/json', data_to_post) - project = unit.active_projects.first - add_auth_header_for user: unit.main_convenor_user - post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post - assert_equal 201, last_response.status, last_response_body - - # test submission generation - task = project.task_for_task_definition(td) - assert task.convert_submission_to_pdf(log_to_stdout: true) - path = task.zip_file_path_for_done_task - assert path - assert File.exist? path - assert File.exist? task.final_pdf_path - - # ensure the notice is not included - reader = PDF::Reader.new(task.final_pdf_path) - assert_not reader.pages[1].text.include? "This file has additional line breaks applied" - - td.destroy - assert_not File.exist? path - unit.destroy! - end - - def test_code_submission_with_long_lines - unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) - td = TaskDefinition.new({ - unit_id: unit.id, - tutorial_stream: unit.tutorial_streams.first, - name: 'Task with super ling lines in code submission', - description: 'Code task', - weighting: 4, - target_grade: 0, - start_date: unit.start_date + 1.week, - target_date: unit.start_date + 2.weeks, - abbreviation: 'Long', - restrict_status_updates: false, - upload_requirements: [ { "key" => 'file0', "name" => 'long.py', "type" => 'code' } ], - plagiarism_warn_pct: 0.8, - is_graded: false, - max_quality_pts: 0 - }) - td.save! - - data_to_post = { - trigger: 'ready_for_feedback' - } - - data_to_post = with_file('test_files/submissions/long.py', 'application/json', data_to_post) - - project = unit.active_projects.first - - add_auth_header_for user: unit.main_convenor_user - - post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post - - assert_equal 201, last_response.status, last_response_body - - # test submission generation - task = project.task_for_task_definition(td) - assert task.convert_submission_to_pdf(log_to_stdout: true) - path = task.zip_file_path_for_done_task - assert path - assert File.exist? path - assert File.exist? task.final_pdf_path - - # ensure the notice is included when rendered files are truncated - reader = PDF::Reader.new(task.final_pdf_path) - assert reader.pages[1].text.include? "This file has additional line breaks applied" - - # submit a normal file and ensure the notice is not included in the PDF - data_to_post = { - trigger: 'ready_for_feedback' - } - - data_to_post = with_file('test_files/submissions/normal.py', 'application/json', data_to_post) - project = unit.active_projects.first - add_auth_header_for user: unit.main_convenor_user - post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post - assert_equal 201, last_response.status, last_response_body - - # test submission generation - task = project.task_for_task_definition(td) - assert task.convert_submission_to_pdf(log_to_stdout: true) - path = task.zip_file_path_for_done_task - assert path - assert File.exist? path - assert File.exist? task.final_pdf_path - - # ensure the notice is not included - reader = PDF::Reader.new(task.final_pdf_path) - assert_not reader.pages[1].text.include? "This file has additional line breaks applied" - - td.destroy - assert_not File.exist? path - unit.destroy! - end - - def test_code_submission_with_long_lines + def test_code_submission_pdf_adds_line_break_notice_only_for_long_lines unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) td = TaskDefinition.new({ unit_id: unit.id, @@ -976,7 +831,10 @@ def test_pdf_creation_fails_on_invalid_pdf rescue StandardError => e task.reload - assert_equal 2, task.comments.count + # The status comment for the move to fix, the automatic resubmission + # extension that comes with it, and the automated comment about the failure + assert_equal 3, task.comments.count + assert_equal 1, task.comments.where(type: 'ExtensionComment').count assert task.comments.last.comment.starts_with?('**Automated Comment**:') assert task.comments.last.comment.include?(e.message.to_s) @@ -1764,4 +1622,438 @@ def test_prerequisite_tasks_change_to_fix_and_resubmit assert_equal TaskStatus.complete, task3.task_status, "Task not Ready for Feedback should not be affected" assert_equal TaskStatus.ready_for_feedback, task4.task_status # Task 4 has no prerequsite links end + + # + # Build a unit with a single task, for the automatic resubmission extension + # tests below. An overdue target date is the case that mattered, because a + # task that is already late stays inside the one week window after it has + # been extended, so every repeat of the assessment used to add another week. + # + def create_task_for_resubmission_extension(weeks_on_resubmit: 1, target_date: Time.zone.now + 2.days) + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0, start_date: Time.zone.now - 6.weeks, end_date: Time.zone.now + 10.weeks) + unit.allow_student_extension_requests = true + unit.extension_weeks_on_resubmit_request = weeks_on_resubmit + unit.save! + + td = TaskDefinition.new({ + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Resubmission task', + description: 'Resubmission task', + weighting: 4, + target_grade: 0, + start_date: unit.start_date, + target_date: target_date, + abbreviation: 'RESUB', + restrict_status_updates: false, + upload_requirements: [ ], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + }) + td.save! + + project = unit.active_projects.first + [unit, td, project.task_for_task_definition(td)] + end + + # Assessing the same submission again must not move the deadline again. + def test_resubmission_extension_granted_once_per_round + unit, _td, task = create_task_for_resubmission_extension(target_date: Time.zone.now - 3.weeks) + tutor = unit.main_convenor_user + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 1, task.reload.extensions, 'The first fix should grant the resubmission extension' + + first_due_date = task.due_date + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 1, task.reload.extensions, 'Assessing the same submission again must not extend again' + assert_equal first_due_date, task.due_date, 'The effective deadline must not move on a repeated assessment' + + task.assess(TaskStatus.discuss, tutor) + assert_equal 1, task.reload.extensions, 'Another resubmission status in the same round must not extend again' + assert_equal first_due_date, task.due_date + + unit.destroy! + end + + # A new submission starts a new round of feedback, which earns its own + # extension. That is the rule the unit has today and it is unchanged. + def test_resubmission_extension_returns_after_a_new_submission + unit, _td, task = create_task_for_resubmission_extension + tutor = unit.main_convenor_user + student = unit.active_projects.first.student + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 1, task.reload.extensions + + # A week later the student resubmits and is sent back to fix it again + travel_to Time.zone.now + 8.days do + task.submit(student) + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 2, task.reload.extensions, 'A new submission earns a new resubmission extension' + end + + unit.destroy! + end + + # The extension has to say why it happened and what triggered it. + def test_resubmission_extension_records_its_reason + unit, _td, task = create_task_for_resubmission_extension + tutor = unit.main_convenor_user + + task.assess(TaskStatus.fix_and_resubmit, tutor) + task.reload + + extension = task.resubmission_extension_comment + assert_not_nil extension, 'The automatic extension should be recorded against the task' + assert_equal 'ExtensionComment', extension.type + assert_equal 1, extension.extension_weeks + assert extension.extension_granted, 'The recorded extension should be marked as granted' + assert_equal TaskStatus.fix_and_resubmit, extension.task_status, 'The status that triggered the extension should be recorded' + assert_equal tutor, extension.assessor + assert extension.assessed? + assert extension.comment.present?, 'The extension should explain itself to the student' + assert extension.extension_response.include?(task.due_date.strftime('%a %b %e')), 'The response should name the new deadline' + + serialized = extension.serialize(tutor) + assert serialized[:resubmission_extension], 'The interface needs to know OnTrack worked this extension out itself' + assert_equal :fix_and_resubmit, serialized[:source_status] + + unit.destroy! + end + + # A larger extension granted later must survive a repeated assessment. + def test_resubmission_extension_does_not_shorten_a_later_extension + unit, _td, task = create_task_for_resubmission_extension(target_date: Time.zone.now - 3.weeks) + tutor = unit.main_convenor_user + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 1, task.reload.extensions + + assert task.grant_extension(tutor, 2), 'A tutor should be able to grant a further extension' + assert_equal 3, task.reload.extensions + later_due_date = task.due_date + + task.assess(TaskStatus.fix_and_resubmit, tutor) + task.reload + assert_equal 3, task.extensions, 'A later extension must not be lost, and must not be added to' + assert_equal later_due_date, task.due_date + + unit.destroy! + end + + # The recursive fix of dependent tasks runs an assessment on each of them. + # Replaying it must not extend those tasks a second time. + def test_recursive_fix_does_not_extend_dependent_tasks_twice + unit = FactoryBot.create(:unit, student_count: 1, task_count: 2) + unit.extension_weeks_on_resubmit_request = 1 + unit.save! + + tutor = FactoryBot.create(:user, :tutor) + unit.employ_staff(tutor, Role.tutor) + + td1 = unit.task_definitions.first + td2 = unit.task_definitions.second + + [td1, td2].each do |td| + td.update!(start_date: Time.zone.now - 6.weeks, target_date: Time.zone.now - 3.weeks, due_date: Time.zone.now + 8.weeks, target_grade: 0) + end + + TaskPrerequisite.create!( + task_definition: td2, + prerequisite: td1, + task_status_id: TaskStatus.ready_for_feedback.id + ) + + project = unit.active_projects.first + task1 = project.task_for_task_definition(td1) + task2 = project.task_for_task_definition(td2) + + task1.update!(task_status: TaskStatus.ready_for_feedback) + task2.update!(task_status: TaskStatus.ready_for_feedback) + + task1.assess(TaskStatus.fix_and_resubmit, tutor, Time.zone.now, true) + assert_equal 1, task1.reload.extensions, 'The assessed task should be extended once' + assert_equal 1, task2.reload.extensions, 'The dependent task should be extended once' + + # Replay the same event. The dependent task is put back to ready for + # feedback so the recursion reaches it again, as a duplicate event would. + task2.update!(task_status: TaskStatus.ready_for_feedback) + task1.assess(TaskStatus.fix_and_resubmit, tutor, Time.zone.now, true) + + assert_equal 1, task1.reload.extensions, 'The assessed task must not be extended twice' + assert_equal 1, task2.reload.extensions, 'The dependent task must not be extended twice' + + unit.destroy! + end + + # The window is measured from the assessment being processed, not from the + # wall clock, so replaying an old event gives the answer it gave then. + def test_resubmission_extension_window_uses_the_assessment_time + unit, _td, task = create_task_for_resubmission_extension + tutor = unit.main_convenor_user + + assert task.resubmission_extension_window_open?(Time.zone.now), 'The deadline is two days away, so the window is open now' + assert_not task.resubmission_extension_window_open?(Time.zone.now - 3.weeks), 'Three weeks ago the deadline was not close' + + task.assess(TaskStatus.fix_and_resubmit, tutor, Time.zone.now - 3.weeks) + assert_equal 0, task.reload.extensions, 'An assessment made when the deadline was far away should not extend it' + + unit.destroy! + end + + # + # Melbourne puts its clocks back at 03:00 on Sunday 5 April 2026 and forward + # at 02:00 on Sunday 4 October 2026, so 2 April and 8 October are +11:00 while + # 9 April and 1 October are +10:00. Those are the four dates the tests below + # use. + # + # Every one of them leaves the application zone alone on purpose. Nothing in + # config/ sets config.time_zone, so that zone is UTC, and the whole point of + # the fix is that the deadline maths no longer depends on it. The zone comes + # off the campus the student is enrolled at. + # + + # The last moment of a given day, anywhere on earth. Fixed offset, so it never + # observes daylight saving itself. + def end_of_day_anywhere_on_earth(year, month, day) + Time.new(year, month, day, 23, 59, 59, '-12:00') + end + + # Put the campuses these tasks belong to onto a real Australian zone, then put + # them back so nothing else in the suite sees the change. + def with_campus_timezone(zone_name, *tasks) + campuses = tasks.map { |task| task.project.campus }.compact.uniq + previous = campuses.map { |campus| [campus, campus.read_attribute(:timezone)] } + + campuses.each { |campus| campus.update!(timezone: zone_name) } + yield + ensure + previous.each { |campus, was| campus.update!(timezone: was) } + end + + # A deadline set at the same time of day on either side of a daylight saving + # change has to land on the day it was set for, and two of them a week apart + # have to stay a week apart. + # + # This used to read the day, month and year straight off the deadline as it + # was loaded, which meant reading them in UTC. 10:30 in Melbourne is the + # previous day in UTC through summer and the same day through winter, so the + # effective deadline jumped a whole day at the boundary. + def test_effective_deadline_does_not_drift_across_a_daylight_saving_boundary + melbourne = ActiveSupport::TimeZone['Australia/Melbourne'] + unit, td, task = create_task_for_resubmission_extension + + with_campus_timezone('Australia/Melbourne', task) do + # The week the clocks go back, then the week they go forward + [[[2026, 4, 2], [2026, 4, 9]], [[2026, 10, 1], [2026, 10, 8]]].each do |first, second| + deadlines = [first, second].map do |year, month, day| + td.update!(target_date: melbourne.local(year, month, day, 10, 30, 0)) + task.reload.effective_deadline + end + + assert_equal end_of_day_anywhere_on_earth(*first), deadlines.first, + "A task due at 10:30 in Melbourne on #{first.join('-')} runs to the end of that day, not the one before" + assert_equal end_of_day_anywhere_on_earth(*second), deadlines.second, + "A task due at 10:30 in Melbourne on #{second.join('-')} runs to the end of that day, not the one before" + assert_equal 7.days.to_i, (deadlines.second - deadlines.first).to_i, + 'Two deadlines a week apart on the campus calendar stay a week apart when the clocks change between them' + end + end + + unit.destroy! + end + + # Seven days has to mean seven days on the campus calendar. The week Melbourne + # moves onto daylight saving is 167 real hours long and the week it moves off + # is 169, so counting a flat 168 moves the edge of the window by an hour. + # + # The assessment time is handed in the way Task#assess gets it. Nothing sets + # config.time_zone, so that is a UTC value, and the whole point is that the + # window is then measured on the campus clock rather than on that one. Feed + # this a Melbourne time instead and it passes either way, because adding a + # duration to a value that is already in the campus zone does the right thing + # on its own and the test proves nothing. + def test_resubmission_extension_window_keeps_its_wall_clock_across_a_daylight_saving_boundary + melbourne = ActiveSupport::TimeZone['Australia/Melbourne'] + unit, _td, task = create_task_for_resubmission_extension + + assert_equal 'UTC', Time.zone.name, 'This test is only meaningful while the application zone is not the campus zone' + + with_campus_timezone('Australia/Melbourne', task) do + # Nine in the morning in Melbourne on the Thursday before the clocks go + # forward, arriving as the UTC instant the application would hand over + forward_from = melbourne.local(2026, 10, 1, 9, 0, 0).in_time_zone(Time.zone) + forward_to = task.resubmission_extension_window_end(forward_from) + + assert_equal 'UTC', forward_from.time_zone.name + assert_equal melbourne.local(2026, 10, 8, 9, 0, 0), forward_to, + 'Seven days after nine in the morning is nine in the morning, in the week the clocks go forward' + assert_equal 167, ((forward_to - forward_from) / 3600.0).round, + 'That week is 167 real hours, so a flat 168 would push the edge of the window an hour late' + + back_from = melbourne.local(2026, 4, 2, 9, 0, 0).in_time_zone(Time.zone) + back_to = task.resubmission_extension_window_end(back_from) + + assert_equal 'UTC', back_from.time_zone.name + assert_equal melbourne.local(2026, 4, 9, 9, 0, 0), back_to, + 'Seven days after nine in the morning is nine in the morning, in the week the clocks go back' + assert_equal 169, ((back_to - back_from) / 3600.0).round, + 'That week is 169 real hours, so a flat 168 would pull the edge of the window an hour early' + end + + unit.destroy! + end + + # The whole thing end to end, over the weekend the clocks actually change. A + # task due Monday 5 October 2026, sent back on Thursday 1 October, has to come + # out due Monday 12 October. Not Sunday the 11th, and not an hour either side + # of the end of the 12th. + def test_resubmission_extension_lands_on_the_right_day_across_a_daylight_saving_boundary + melbourne = ActiveSupport::TimeZone['Australia/Melbourne'] + unit, td, task = create_task_for_resubmission_extension + tutor = unit.main_convenor_user + + with_campus_timezone('Australia/Melbourne', task) do + td.update!(target_date: melbourne.local(2026, 10, 5, 10, 30, 0)) + task.reload + + assert_equal end_of_day_anywhere_on_earth(2026, 10, 5), task.effective_deadline + + # Melbourne moves onto daylight saving on the Sunday in between + task.assess(TaskStatus.fix_and_resubmit, tutor, melbourne.local(2026, 10, 1, 9, 0, 0)) + task.reload + + assert_equal 1, task.extensions, 'A task due in four days should get the one week the unit grants' + assert_equal end_of_day_anywhere_on_earth(2026, 10, 12), task.effective_deadline, + 'One week after Monday the 5th is Monday the 12th, and the clock change must not make it the 11th' + assert_equal Date.new(2026, 10, 12), task.due_date.to_date + + # And the guard still holds on the far side of the change + task.assess(TaskStatus.fix_and_resubmit, tutor, melbourne.local(2026, 10, 6, 9, 0, 0)) + task.reload + + assert_equal 1, task.extensions, 'Reassessing the same submission after the clocks change must not extend it again' + assert_equal end_of_day_anywhere_on_earth(2026, 10, 12), task.effective_deadline + end + + unit.destroy! + end + + # "Automatic" already meant something else on ExtensionComment - a request a + # student made that the unit approved without a person weighing it up. The + # extension OnTrack works out for itself is a different thing and answers to a + # different name, so a reader cannot take one for the other. + def test_a_student_request_is_not_reported_as_a_resubmission_extension + unit, _td, task = create_task_for_resubmission_extension(target_date: Time.zone.now - 3.weeks) + convenor = unit.main_convenor_user + student = unit.active_projects.first.student + + requested = task.apply_for_extension(student, 'I have been unwell all week', 1) + task.reload + + assert requested.assessed?, 'The unit approves requests inside the deadline without asking anyone' + assert requested.extension_granted + assert_not requested.resubmission_extension?, 'A student asking for time is not something OnTrack worked out itself' + assert_not requested.serialize(convenor)[:resubmission_extension] + assert_nil requested.serialize(convenor)[:source_status] + + task.assess(TaskStatus.fix_and_resubmit, convenor) + task.reload + worked_out = task.resubmission_extension_comment + + assert_not_nil worked_out, 'Sending the task back near the deadline should still earn its own extension' + assert worked_out.resubmission_extension?, 'That one carries the status that triggered it' + assert_equal :fix_and_resubmit, worked_out.serialize(convenor)[:source_status] + assert_equal 2, task.extensions, 'The two are counted separately' + + unit.destroy! + end + + # The extension only applies when the deadline is close. + def test_no_resubmission_extension_when_the_deadline_is_far_away + unit, _td, task = create_task_for_resubmission_extension(target_date: Time.zone.now + 4.weeks) + tutor = unit.main_convenor_user + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 0, task.reload.extensions, 'A task due in four weeks should not be extended' + assert_nil task.resubmission_extension_comment + + unit.destroy! + end + + # Units that turn the automatic extension off must not get one. + def test_no_resubmission_extension_when_the_unit_grants_zero_weeks + unit, _td, task = create_task_for_resubmission_extension(weeks_on_resubmit: 0) + tutor = unit.main_convenor_user + + task.assess(TaskStatus.fix_and_resubmit, tutor) + assert_equal 0, task.reload.extensions + assert_nil task.resubmission_extension_comment + + unit.destroy! + end + + # A failed archive write must not destroy the previously accepted submission. + # compress_new_to_done used to delete the done zip before rebuilding it, so a + # raise part way through the build left the task with no readable submission at + # all. The stub below stands in for any failure while writing the new archive. + def test_compress_new_to_done_keeps_the_previous_zip_when_the_write_fails + unit = Unit.first + td = TaskDefinition.new( + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Atomic done zip', + description: 'atomic done zip', + weighting: 4, + target_grade: 0, + start_date: unit.start_date + 1.week, + target_date: unit.start_date + 2.weeks, + abbreviation: 'TaskAtomicDoneZip', + restrict_status_updates: false, + upload_requirements: [{ 'key' => 'file0', 'name' => 'A Document', 'type' => 'document' }], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + ) + td.save! + + task = unit.active_projects.first.task_for_task_definition(td) + done_zip = task.zip_file_path_for_done_task + + place_one_document = lambda do + new_dir = task.student_work_dir(:new, true) + FileUtils.cp(test_file_path('submissions/1.2P.pdf'), "#{new_dir}000-document.pdf") + end + + # First, a real submission so there is a previously accepted zip on disk. + place_one_document.call + assert task.compress_new_to_done, 'the first compress should succeed' + assert File.exist?(done_zip), 'the done zip should exist after a successful compress' + original_bytes = File.binread(done_zip) + assert(Zip::File.open(done_zip) { |z| z.entries.any? }, 'the done zip should be a readable archive') + assert_empty Dir.glob("#{done_zip}.tmp-*"), 'a successful compress must not leave a temporary archive' + + # Now a second submission whose archive write fails part way through. The stub + # creates the temporary archive first, then raises, so it also exercises the + # cleanup of the half-written temp file. + place_one_document.call + partial_write = lambda do |path, *_rest| + File.binwrite(path, 'partial archive bytes') + raise 'simulated failure while writing the new archive' + end + Zip::File.stub(:open, partial_write) do + assert_raises(RuntimeError) { task.compress_new_to_done } + end + + # The previously accepted submission must be untouched, not deleted or corrupted. + assert File.exist?(done_zip), 'the previous done zip must survive a failed write' + assert_equal original_bytes, File.binread(done_zip), 'the previous done zip must be byte-for-byte unchanged' + assert(Zip::File.open(done_zip) { |z| z.entries.any? }, 'the previous done zip must still be readable') + assert_empty Dir.glob("#{done_zip}.tmp-*"), 'the failed write must not leak a temporary archive' + + td.destroy + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 4cb9d8ac8f..4d405cdb9a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -17,12 +17,15 @@ class UserTest < ActiveSupport::TestCase nickname: 'Test', role_id: 1, email: 'test@test.org', - username: 'metoo', - password: 'password', - password_confirmation: 'password' + username: 'metoo' } - User.create!(profile) - assert User.last, profile + user = User.new(profile) + user.password = 'password' + user.save! + + assert_equal profile.stringify_keys, user.attributes.slice(*profile.stringify_keys.keys) + assert user.authenticate?('password') + assert User.last.display_peer_progress? end def test_user_is_valid @@ -46,4 +49,90 @@ def test_can_create_multiple_auth_tokens t2 = user.generate_authentication_token! assert_not_equal t1, t2 end + + def test_valid_theme_preferences + [nil, 'light', 'dark', 'system'].each do |theme| + user = FactoryBot.build(:user, theme_preference: theme) + assert user.valid?, "expected #{theme.inspect} to be a valid theme_preference" + end + end + + def test_invalid_theme_preference + user = FactoryBot.build(:user, theme_preference: 'sepia') + refute user.valid? + end + + def test_theme_preference_timestamp_tracks_actual_preference_changes + user = FactoryBot.create(:user) + + assert_nil user.theme_preference + assert_nil user.theme_preference_updated_at + + first_choice_at = Time.zone.parse('2026-08-30 10:00:00 UTC') + travel_to first_choice_at do + user.update!(theme_preference: 'dark') + end + assert_equal first_choice_at, user.theme_preference_updated_at + + travel_to first_choice_at + 30.minutes do + user.update!(theme_preference: 'dark') + end + assert_equal first_choice_at, user.theme_preference_updated_at, + 'model writes of the same value are not API synchronization writes' + + travel_to first_choice_at + 1.hour do + user.update!(nickname: 'Still dark') + end + assert_equal first_choice_at, user.theme_preference_updated_at, + 'unrelated updates must not make the preference look newer' + + second_choice_at = first_choice_at + 2.hours + travel_to second_choice_at do + user.update!(theme_preference: 'light') + end + assert_equal second_choice_at, user.theme_preference_updated_at + end + + def test_clearing_theme_preference_restores_the_never_chosen_state + user = FactoryBot.create(:user, theme_preference: 'dark') + assert_not_nil user.theme_preference_updated_at + + user.update!(theme_preference: nil) + + assert_nil user.theme_preference + assert_nil user.theme_preference_updated_at + end + + def test_display_name_uses_nickname_when_present + user = FactoryBot.build( + :user, + first_name: 'Elizabeth', + last_name: 'Smith', + nickname: 'Liz' + ) + + assert_equal 'Liz Smith', user.display_name + end + + def test_display_name_falls_back_to_legal_first_name + user = FactoryBot.build( + :user, + first_name: 'Elizabeth', + last_name: 'Smith', + nickname: nil + ) + + assert_equal 'Elizabeth Smith', user.display_name + end + + def test_display_name_ignores_blank_nickname + user = FactoryBot.build( + :user, + first_name: 'Elizabeth', + last_name: 'Smith', + nickname: ' ' + ) + + assert_equal 'Elizabeth Smith', user.display_name + end end From 8a5716ca389452d9c7895862b9f6cc444f21c550 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Mon, 28 Sep 2026 00:29:19 +1000 Subject: [PATCH 2/2] feat(core): bring in org api PR 178 for the core files Brings ontrack-features-t2-2026/doubtfire-api#178 to the files this PR already carries, so every file stays in exactly one PR. It carries the Unit Hub and digest preference fields on the user API, and the automated comment flag. --- .ci-setup/crontab | 7 +- app/api/entities/comment_entity.rb | 6 ++ app/api/entities/user_entity.rb | 5 ++ app/api/users_api.rb | 48 +++++++++++++ app/models/comments/task_comment.rb | 28 ++++++++ test/api/users_test.rb | 98 +++++++++++++++++++++++++++ test/models/automated_comment_test.rb | 38 +++++++++++ 7 files changed, 229 insertions(+), 1 deletion(-) create mode 100644 test/models/automated_comment_test.rb diff --git a/.ci-setup/crontab b/.ci-setup/crontab index 0030ad8035..eb686c6ad6 100644 --- a/.ci-setup/crontab +++ b/.ci-setup/crontab @@ -5,5 +5,10 @@ PATH=/tmp/texlive/bin/x86_64-linux:/tmp/texlive/bin/aarch64-linux:/usr/local/bun 10,15,20,25,30,35,40,45,50,55 * * * * /doubtfire/lib/shell/generate_pdfs.sh 0,10,20,30,40,50 * * * * /doubtfire/lib/shell/send_overseer_notifications.sh 0 8 * * * /doubtfire/lib/shell/portfolio_autogen_check.sh -0 7 * * 1 /doubtfire/lib/shell/send_weekly_emails.sh +# The weekly summary email ran here, Monday at 7am. The recurring summary is the +# student digest now, three entries in config/schedule.yml, one per cadence a +# student can pick, so it is scheduled once and it runs anywhere Sidekiq runs +# rather than only in this container. The per-unit mail this line used to send +# is still there and is still `rake mailer:send_status_emails`, via +# lib/shell/send_weekly_emails.sh, but it is on no schedule now. 0 1 * * * /doubtfire/lib/shell/sync_enrolments.sh diff --git a/app/api/entities/comment_entity.rb b/app/api/entities/comment_entity.rb index 3a8e506d07..5101fd3fc4 100644 --- a/app/api/entities/comment_entity.rb +++ b/app/api/entities/comment_entity.rb @@ -15,6 +15,12 @@ class CommentEntity < Grape::Entity data.new_for?(options[:current_user]) end end + # Written by OnTrack rather than by the author named below. Clients show + # these differently so a person is not credited with something they did not + # write. + expose :automated do |data, _options| + data.respond_to?(:automated?) && data.automated? + end expose :reply_to_id expose :attachment_file_name, if: ->(data, _) { data.attachment? } expose :attachment_mime_type, if: ->(data, _) { data.attachment? } diff --git a/app/api/entities/user_entity.rb b/app/api/entities/user_entity.rb index 2a8306f2c1..a200ece847 100644 --- a/app/api/entities/user_entity.rb +++ b/app/api/entities/user_entity.rb @@ -11,6 +11,11 @@ class UserEntity < Grape::Entity expose :receive_task_notifications, unless: :minimal expose :receive_portfolio_notifications, unless: :minimal expose :receive_feedback_notifications, unless: :minimal + expose :receive_unit_hub_notifications, unless: :minimal + expose :receive_unit_hub_email_notifications, unless: :minimal + expose :receive_unit_hub_push_notifications, unless: :minimal + expose :receive_unit_hub_session_reminders, unless: :minimal + expose :digest_frequency, unless: :minimal expose :display_peer_progress, unless: :minimal expose :opt_in_to_research, unless: :minimal expose :has_run_first_time_setup, unless: :minimal diff --git a/app/api/users_api.rb b/app/api/users_api.rb index b40943961c..71f79d2fd7 100644 --- a/app/api/users_api.rb +++ b/app/api/users_api.rb @@ -77,6 +77,11 @@ class UsersApi < Grape::API optional :receive_task_notifications, type: Boolean, desc: 'Allow user to be sent task notifications' optional :receive_portfolio_notifications, type: Boolean, desc: 'Allow user to be sent portfolio notifications' optional :receive_feedback_notifications, type: Boolean, desc: 'Allow user to be sent feedback notifications' + optional :receive_unit_hub_notifications, type: Boolean, desc: 'Show Unit Hub announcement and session updates in the app' + optional :receive_unit_hub_email_notifications, type: Boolean, desc: 'Also email Unit Hub updates' + optional :receive_unit_hub_push_notifications, type: Boolean, desc: 'Also push Unit Hub updates to subscribed browsers' + optional :receive_unit_hub_session_reminders, type: Boolean, desc: 'Remind the user shortly before Unit Hub sessions start' + optional :digest_frequency, type: String, values: User::DIGEST_FREQUENCIES, desc: 'How often to send the unit summary email [off, daily, weekly, monthly]' optional :display_peer_progress, type: Boolean, desc: 'Display anonymous peer progress information' optional :opt_in_to_research, type: Boolean, desc: 'Allow user to opt in to research conducted by Doubtfire' optional :has_run_first_time_setup, type: Boolean, desc: 'Whether or not user has run first-time setup' @@ -92,10 +97,23 @@ class UsersApi < Grape::API %i[receive_task_notifications receive_portfolio_notifications receive_feedback_notifications].each do |pref| params[:user][pref] = true if params[:user].key?(pref) && params[:user][pref].nil? end + # The Unit Hub columns are NOT NULL, so a null goes back to each one's own + # default: on for the in-app bell, off for the three opt-ins. + { + receive_unit_hub_notifications: true, + receive_unit_hub_email_notifications: false, + receive_unit_hub_push_notifications: false, + receive_unit_hub_session_reminders: false + }.each do |pref, default| + params[:user][pref] = default if params[:user].key?(pref) && params[:user][pref].nil? + end if params[:user].key?(:display_peer_progress) && params[:user][:display_peer_progress].nil? params[:user][:display_peer_progress] = true end + # NOT NULL with a default, so a null means "put it back to the default" + # rather than "clear it", matching the Unit Hub preferences above. + params[:user][:digest_frequency] = 'weekly' if params[:user].key?(:digest_frequency) && params[:user][:digest_frequency].nil? # can only modify if current_user.id is same as :id provided # (i.e., user wants to update their own data) or if update_user token @@ -114,6 +132,18 @@ class UsersApi < Grape::API error!({ error: 'Sign-in email is managed by your institution and cannot be changed here.' }, 422) end + # Names come from the same asserted identity as the sign-in email. Without + # this a student could rename themselves permanently, because SAML/AAF only + # writes first/last name when the account is first created, so a later + # sign-in never restores what the institution holds. + name_changed = + (params[:user].key?(:first_name) && params[:user][:first_name].to_s != user.first_name.to_s) || + (params[:user].key?(:last_name) && params[:user][:last_name].to_s != user.last_name.to_s) + + if name_changed && !AuthenticationHelpers.db_auth? + error!({ error: 'Your name is managed by your institution and cannot be changed here.' }, 422) + end + if params[:user].key?(:student_id) && params[:user][:student_id].to_s != user.student_id.to_s && (change_self || !AuthenticationHelpers.db_auth?) @@ -131,6 +161,11 @@ class UsersApi < Grape::API :receive_task_notifications, :receive_portfolio_notifications, :receive_feedback_notifications, + :receive_unit_hub_notifications, + :receive_unit_hub_email_notifications, + :receive_unit_hub_push_notifications, + :receive_unit_hub_session_reminders, + :digest_frequency, :display_peer_progress, :opt_in_to_research, :has_run_first_time_setup, @@ -142,6 +177,19 @@ class UsersApi < Grape::API # field instead of rejecting the rest of an otherwise valid update. user_parameters.delete(:theme_preference) unless change_self + # The Unit Hub opt-ins and the digest decide what lands in the user's own + # inbox, so only the user can turn them on or off. Staff with update_user + # can still edit the rest of the profile. + unless change_self + %i[ + receive_unit_hub_notifications + receive_unit_hub_email_notifications + receive_unit_hub_push_notifications + receive_unit_hub_session_reminders + digest_frequency + ].each { |pref| user_parameters.delete(pref) } + end + user.role = Role.student if user.role.nil? old_role = user.role diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index 5108dc66ea..8447618394 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -8,6 +8,34 @@ class TaskComment < ApplicationRecord include FileHelper include AuthorisationHelpers + # OnTrack writes some comments itself: the automatic extension when a task is + # set to Fix and Resubmit near its deadline, and the notice when a submission + # fails to process. They are stored against the tutor for that task, because a + # comment needs an author and a recipient, but they are not something a person + # wrote. The marker is the text prefix, which several queries here already + # match on; this keeps the four copies of that literal in one place. + AUTOMATED_PREFIXES = ['**Automated Message:', '**Automated Comment**:'].freeze + + # True when OnTrack wrote this comment rather than a person. + def automated? + text = comment.to_s + AUTOMATED_PREFIXES.any? { |prefix| text.start_with?(prefix) } + end + + # The whole marker, including the bold that closes it. AUTOMATED_PREFIXES is + # deliberately shorter than this: it matches the same literal the existing + # `LIKE '**Automated Message:%'` queries use, which stops before the closing + # asterisks. Stripping needs the rest or it leaves them behind. + AUTOMATED_PREFIX_PATTERN = /\A\*\*Automated (?:Message:\*\*|Comment\*\*:)\s*/ + + # The comment without its marker, for a client that says "automated" some + # other way and would otherwise show the label twice. + def comment_without_automated_prefix + return comment unless automated? + + comment.to_s.sub(AUTOMATED_PREFIX_PATTERN, '').strip + end + belongs_to :task, optional: false # Foreign key belongs_to :user, optional: false has_one :unit, through: :task diff --git a/test/api/users_test.rb b/test/api/users_test.rb index 0cd53ff217..800fbaeb45 100644 --- a/test/api/users_test.rb +++ b/test/api/users_test.rb @@ -467,6 +467,27 @@ def test_non_self_update_ignores_theme_preference_and_omits_it_from_response assert_not last_response_body.key?('theme_preference_updated_at') end + def test_non_self_update_ignores_unit_hub_and_digest_preferences + current_user = User.first + other_user = User.second + other_user.update!(digest_frequency: 'off', receive_unit_hub_email_notifications: false) + add_auth_header_for(user: current_user) + + put_json "/api/users/#{other_user.id}", { + user: { + nickname: 'Updated by staff', + digest_frequency: 'daily', + receive_unit_hub_email_notifications: true + } + } + + assert_equal 200, last_response.status + other_user.reload + assert_equal 'Updated by staff', other_user.nickname + assert_equal 'off', other_user.digest_frequency + assert_not other_user.receive_unit_hub_email_notifications + end + def test_put_invalid_theme_preference_keeps_the_existing_choice_and_timestamp user = User.first chosen_at = Time.zone.parse('2026-08-30 10:00:00 UTC') @@ -591,6 +612,83 @@ def test_sso_controlled_email_rejects_self_and_admin_forgery end end + def test_sso_controlled_name_rejects_self_and_admin_forgery + with_auth_method(:saml) do + user = FactoryBot.create(:user, first_name: 'Institutional', last_name: 'Record') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { user: { first_name: 'Forged' } } + assert_equal 422, last_response.status + assert_equal 'Your name is managed by your institution and cannot be changed here.', last_response_body['error'] + assert_equal 'Institutional', user.reload.first_name + + put_json "/api/users/#{user.id}", { user: { last_name: 'Forged' } } + assert_equal 422, last_response.status + assert_equal 'Record', user.reload.last_name + + admin = FactoryBot.create(:user, :admin) + add_auth_header_for(user: admin) + put_json "/api/users/#{user.id}", { user: { first_name: 'Admin forged' } } + assert_equal 422, last_response.status + assert_equal 'Institutional', user.reload.first_name + end + end + + def test_sso_update_that_resends_the_same_name_is_accepted + with_auth_method(:saml) do + user = FactoryBot.create(:user, first_name: 'Institutional', last_name: 'Record') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { + user: { + first_name: 'Institutional', + last_name: 'Record', + nickname: 'Preferred' + } + } + + assert_equal 200, last_response.status + assert_equal 'Preferred', user.reload.nickname + assert_equal 'Institutional', user.first_name + assert_equal 'Record', user.last_name + end + end + + def test_sso_user_can_always_change_their_preferred_name + with_auth_method(:saml) do + user = FactoryBot.create(:user, first_name: 'Institutional', nickname: 'Old') + add_auth_header_for(user: user) + + put_json "/api/users/#{user.id}", { user: { nickname: 'New preferred' } } + + assert_equal 200, last_response.status + assert_equal 'New preferred', user.reload.nickname + assert_equal 'Institutional', user.first_name + end + end + + def test_local_accounts_can_still_change_their_name + with_auth_method(:database) do + local_user = FactoryBot.create(:user, first_name: 'Local', last_name: 'Account') + add_auth_header_for(user: local_user) + + put_json "/api/users/#{local_user.id}", { + user: { first_name: 'Changed', last_name: 'Name' } + } + + assert_equal 200, last_response.status + assert_equal 'Changed', local_user.reload.first_name + assert_equal 'Name', local_user.last_name + + admin = FactoryBot.create(:user, :admin) + add_auth_header_for(user: admin) + put_json "/api/users/#{local_user.id}", { user: { first_name: 'Admin maintained' } } + + assert_equal 200, last_response.status + assert_equal 'Admin maintained', local_user.reload.first_name + end + end + def test_sso_user_can_still_save_preferred_name_and_preferences with_auth_method(:saml) do user = FactoryBot.create(:user, email: 'institutional@example.edu') diff --git a/test/models/automated_comment_test.rb b/test/models/automated_comment_test.rb new file mode 100644 index 0000000000..08ed8a4ae2 --- /dev/null +++ b/test/models/automated_comment_test.rb @@ -0,0 +1,38 @@ +require 'test_helper' + +# OnTrack writes some comments itself. They are stored against the tutor for the +# task, because a comment needs an author, so without a marker they read as +# something that person wrote. +class AutomatedCommentTest < ActiveSupport::TestCase + def build_comment(text) + comment = TaskComment.new + comment.comment = text + comment + end + + def test_recognises_both_markers_ontrack_writes + assert build_comment('**Automated Message:** This task was extended.').automated? + assert build_comment('**Automated Comment**: Something went wrong.').automated? + end + + def test_a_person_writing_about_automation_is_not_automated + refute build_comment('The **Automated Message:** you got was wrong, sorry.').automated? + refute build_comment('Nice work on this one.').automated? + refute build_comment('').automated? + refute build_comment(nil).automated? + end + + # The marker closes with bold that AUTOMATED_PREFIXES stops short of, because + # that constant matches the same literal the existing LIKE queries use. A + # strip that only removed the prefix left the closing asterisks behind. + def test_strips_the_whole_marker_including_the_bold_that_closes_it + comment = build_comment('**Automated Message:** This task was extended by 1 week.') + assert_equal 'This task was extended by 1 week.', comment.comment_without_automated_prefix + + other = build_comment('**Automated Comment**: Something went wrong.') + assert_equal 'Something went wrong.', other.comment_without_automated_prefix + + written = build_comment('Nice work on this one.') + assert_equal 'Nice work on this one.', written.comment_without_automated_prefix + end +end