diff --git a/.ci-setup/crontab b/.ci-setup/crontab index b1298d5e39..eb686c6ad6 100644 --- a/.ci-setup/crontab +++ b/.ci-setup/crontab @@ -4,7 +4,11 @@ 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 +# 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/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..5101fd3fc4 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" @@ -15,7 +15,18 @@ 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? } + 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..a200ece847 100644 --- a/app/api/entities/user_entity.rb +++ b/app/api/entities/user_entity.rb @@ -7,11 +7,39 @@ 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 :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 + # 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..71f79d2fd7 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,43 @@ 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' + 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 + # 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 @@ -76,6 +121,35 @@ 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 + + # 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?) + 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 +161,35 @@ 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 + :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 + + # 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 @@ -115,13 +214,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 +230,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..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 @@ -17,6 +45,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 +60,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 +90,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 +101,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 +129,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 +176,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..800fbaeb45 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,160 @@ 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_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') + 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 +564,247 @@ 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_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') + 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/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 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