From e02bb60896dc05be44520cdee7c638a527ecdc99 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:49:56 +1000 Subject: [PATCH 1/3] feat: project portfolio lock --- app/api/discussion_comment_api.rb | 4 +- app/api/entities/project_entity.rb | 13 ++ app/api/entities/unit_entity.rb | 11 + app/api/projects_api.rb | 19 +- app/api/submission/portfolio_api.rb | 33 ++- app/api/task_comments_api.rb | 3 +- app/api/tasks_api.rb | 4 + app/api/units_api.rb | 16 ++ app/helpers/authorisation_helpers.rb | 39 ++++ app/models/comments/task_comment.rb | 19 ++ app/models/group.rb | 36 ++-- app/models/group_submission.rb | 44 ++-- app/models/overseer_assessment.rb | 2 +- .../project_compile_portfolio_module.rb | 8 + app/models/project.rb | 21 ++ app/models/task.rb | 32 ++- app/models/task_definition.rb | 4 + app/models/unit.rb | 78 ++++++++ app/sidekiq/accept_overseer_job.rb | 12 +- app/sidekiq/execute_communication_set_job.rb | 11 + ...resh_moderation_feedback_timestamps_job.rb | 2 +- ..._portfolio_deadline_and_submission_lock.rb | 7 + db/schema.rb | 6 +- .../api/portfolio_submission_lock_api_test.rb | 137 +++++++++++++ test/api/projects_api_test.rb | 15 +- test/api/units_api_test.rb | 8 +- test/models/portfolio_submission_lock_test.rb | 188 ++++++++++++++++++ test/models/unit_model_test.rb | 23 +++ .../execute_communication_set_job_test.rb | 36 +++- 29 files changed, 778 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb create mode 100644 test/api/portfolio_submission_lock_api_test.rb create mode 100644 test/models/portfolio_submission_lock_test.rb diff --git a/app/api/discussion_comment_api.rb b/app/api/discussion_comment_api.rb index ffd70117fc..d619d7cc92 100644 --- a/app/api/discussion_comment_api.rb +++ b/app/api/discussion_comment_api.rb @@ -66,7 +66,9 @@ class DiscussionCommentApi < Grape::API if project.has_task_for_task_definition? task_definition task = project.task_for_task_definition(task_definition) discussion_comment = task.all_comments.find(params[:task_comment_id]).becomes(DiscussionComment) - discussion_comment.mark_discussion_started + # Opening a prompt is read-only while a submitted portfolio freezes the + # project. Keep the download available without changing discussion state. + discussion_comment.mark_discussion_started unless project.portfolio_locked? prompt_path = discussion_comment.attachment_path(prompt_number) diff --git a/app/api/entities/project_entity.rb b/app/api/entities/project_entity.rb index ebe150267f..f75e14b01d 100644 --- a/app/api/entities/project_entity.rb +++ b/app/api/entities/project_entity.rb @@ -16,6 +16,19 @@ class ProjectEntity < Grape::Entity expose :portfolio_files, unless: :summary_only expose :compile_portfolio, unless: :summary_only expose :portfolio_available + # Grape passes entity options to exposure procs, so Symbol#to_proc is not compatible here. + # rubocop:disable Style/SymbolProc + expose :portfolio_locked do |project| + project.portfolio_locked? + end + expose :effective_portfolio_deadline, expose_nil: true do |project| + project.effective_portfolio_deadline&.iso8601 + end + expose :effective_portfolio_deadline_timezone, expose_nil: true + expose :portfolio_deadline_passed do |project| + project.portfolio_deadline_passed? + end + # rubocop:enable Style/SymbolProc expose :portfolio_submission_date, if: :for_staff expose :uses_draft_learning_summary, unless: :summary_only diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index c820f2e362..cce564d6b1 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -6,6 +6,10 @@ class UnitEntity < Grape::Entity date.strftime('%Y-%m-%d') end + format_with(:local_datetime) do |date| + date&.strftime('%Y-%m-%dT%H:%M') + end + def is_staff?(my_role) [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? end @@ -63,6 +67,13 @@ def can_read_unit_config?(my_role) expose :allow_flexible_dates, unless: :summary_only expose :mark_late_submissions_as_assess_in_portfolio, unless: :summary_only + with_options(unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }) do + expose :portfolio_deadline, format_with: :local_datetime, expose_nil: true + expose :portfolio_deadline_per_campus + expose :portfolio_deadline_campus_id, expose_nil: true + expose :lock_project_on_portfolio_submission + end + expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only diff --git a/app/api/projects_api.rb b/app/api/projects_api.rb index 61a66c1fdd..36f552b1b0 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -165,7 +165,24 @@ class ProjectsApi < Grape::API project.save end - Entities::ProjectEntity.represent(project, only: [:campus_id, :enrolled, :target_grade, :submitted_grade, :compile_portfolio, :portfolio_available, :uses_draft_learning_summary, :stats], for_student: for_student) + Entities::ProjectEntity.represent( + project, + only: [ + :campus_id, + :enrolled, + :target_grade, + :submitted_grade, + :compile_portfolio, + :portfolio_available, + :portfolio_locked, + :effective_portfolio_deadline, + :effective_portfolio_deadline_timezone, + :portfolio_deadline_passed, + :uses_draft_learning_summary, + :stats + ], + for_student: for_student + ) end # put desc 'Enrol a student in a unit, creating them a project' diff --git a/app/api/submission/portfolio_api.rb b/app/api/submission/portfolio_api.rb index aec64c65e1..1e934d2a7e 100644 --- a/app/api/submission/portfolio_api.rb +++ b/app/api/submission/portfolio_api.rb @@ -45,22 +45,41 @@ class PortfolioApi < Grape::API optional :idx, type: Integer, desc: 'The index of the file' optional :kind, type: String, desc: 'The kind of file being removed: document, code, or image' optional :name, type: String, desc: 'Name of file to remove' + optional :confirm_late, type: Boolean, default: false, desc: 'Confirm deletion after the effective portfolio deadline' end delete '/submission/project/:id/portfolio' do project = Project.find(params[:id]) + deleting_whole_portfolio = params[:idx].nil? && params[:name].nil? && params[:kind].nil? + + if deleting_whole_portfolio + unit_role = project.unit.unit_role_for(current_user) + can_delete = project.user_id == current_user.id || unit_role&.role_id == Role.convenor_id + unless can_delete + error!({ error: "Not authorised to delete portfolio for project '#{params[:id]}'" }, 403) + end + if project.compile_portfolio? + error!({ error: 'The portfolio is still compiling. Wait for compilation to finish before deleting it.' }, 409) + end + if project.portfolio_deadline_passed? && !params[:confirm_late] + error!({ + error: 'Deleting this portfolio requires confirmation because the effective portfolio deadline has passed.', + portfolio_late_confirmation_required: true, + effective_portfolio_deadline: project.effective_portfolio_deadline.iso8601, + effective_portfolio_deadline_timezone: project.effective_portfolio_deadline_timezone + }, 409) + end - unless authorise? current_user, project, :make_submission - error!({ error: "Not authorised to alter portfolio for project '#{params[:id]}'" }, 401) - end - - # Remove file or portfolio? - if params[:idx].nil? && params[:name].nil? && params[:kind].nil? project.update!({ portfolio_submission_date: nil, - portfolio_production_date: nil + portfolio_production_date: nil, + compile_portfolio: false }) project.remove_portfolio # returns details of file elsif !(params[:idx].nil? || params[:name].nil? || params[:kind].nil?) + unless authorise? current_user, project, :make_submission + error!({ error: "Not authorised to alter portfolio for project '#{params[:id]}'" }, 403) + end + idx = params[:idx] name = params[:name] kind = params[:kind] diff --git a/app/api/task_comments_api.rb b/app/api/task_comments_api.rb index cfe2500a87..75d843224a 100644 --- a/app/api/task_comments_api.rb +++ b/app/api/task_comments_api.rb @@ -259,7 +259,8 @@ class TaskCommentsApi < Grape::API project = Project.find(params[:project_id]) task_definition = project.unit.task_definitions.find(params[:task_definition_id]) - unless authorise? current_user, project, :make_submission + # Read-receipt state remains editable even when portfolio evidence is frozen. + unless authorise? current_user, project, :get error!({ error: 'Not authorised to mark comment as unread' }, 403) end diff --git a/app/api/tasks_api.rb b/app/api/tasks_api.rb index acb4e02290..7c7364fcc1 100644 --- a/app/api/tasks_api.rb +++ b/app/api/tasks_api.rb @@ -257,6 +257,10 @@ class TasksApi < Grape::API post '/projects/:id/task_def_id/:task_definition_id/check_in' do project = Project.find(params[:id]) + if project.portfolio_locked? + error!({ error: 'This project is locked because its portfolio has been submitted.' }, 403) + end + unless authorise?(current_user, project, :assess) error!({ error: 'You do not have permission to assess this task.' }, 403) end diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 89e0105eee..2eefe817d6 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -85,6 +85,10 @@ class UnitsApi < Grape::API optional :enable_sync_enrolments, type: Boolean, desc: 'Sync student enrolments automatically if supported by deployment' optional :draft_task_definition_id, type: Integer, desc: 'Indicates the ID of the task definition used as the "draft learning summary task"' optional :portfolio_auto_generation_date, type: Date, desc: 'Indicates a date where student portfolio will automatically compile' + optional :portfolio_deadline, type: String, desc: 'Portfolio deadline entered as YYYY-MM-DDTHH:mm local time' + optional :portfolio_deadline_per_campus, type: Boolean, desc: 'Apply the local deadline in each student campus timezone' + optional :portfolio_deadline_campus_id, type: Integer, desc: 'Campus timezone used for all students when not applying per campus' + optional :lock_project_on_portfolio_submission, type: Boolean, desc: 'Freeze portfolio evidence while a portfolio is compiling or available' optional :allow_flexible_dates, type: Boolean, desc: 'Can turn on/off flexible dates for tasks in this unit' optional :allow_student_extension_requests, type: Boolean, desc: 'Can turn on/off student extension requests' optional :allow_student_change_tutorial, type: Boolean, desc: 'Can turn on/off student ability to change tutorials' @@ -130,6 +134,10 @@ class UnitsApi < Grape::API :enable_sync_enrolments, :draft_task_definition_id, :portfolio_auto_generation_date, + :portfolio_deadline, + :portfolio_deadline_per_campus, + :portfolio_deadline_campus_id, + :lock_project_on_portfolio_submission, :allow_flexible_dates, :allow_student_extension_requests, :extension_weeks_on_resubmit_request, @@ -187,6 +195,10 @@ class UnitsApi < Grape::API optional :allow_student_extension_requests, type: Boolean, desc: 'Can turn on/off student extension requests', default: true optional :extension_weeks_on_resubmit_request, type: Integer, desc: 'Determines the number of weeks extension on a resubmit request', default: 1 optional :portfolio_auto_generation_date, type: Date, desc: 'Indicates a date where student portfolio will automatically compile' + optional :portfolio_deadline, type: String, desc: 'Portfolio deadline entered as YYYY-MM-DDTHH:mm local time' + optional :portfolio_deadline_per_campus, type: Boolean, default: true + optional :portfolio_deadline_campus_id, type: Integer + optional :lock_project_on_portfolio_submission, type: Boolean, default: false optional :allow_student_change_tutorial, type: Boolean, desc: 'Can turn on/off student ability to change tutorials', default: true optional :feedback_warning_threshold_days, type: Integer, desc: 'Number of days since a submission without feedback before its highlighted in the tutors inbox' optional :feedback_overflow_threshold_days, type: Integer, desc: 'Number of days since a submission without feedback before its added to overflow marking' @@ -229,6 +241,10 @@ class UnitsApi < Grape::API :allow_student_extension_requests, :extension_weeks_on_resubmit_request, :portfolio_auto_generation_date, + :portfolio_deadline, + :portfolio_deadline_per_campus, + :portfolio_deadline_campus_id, + :lock_project_on_portfolio_submission, :allow_student_change_tutorial, :feedback_warning_threshold_days, :feedback_overflow_threshold_days, diff --git a/app/helpers/authorisation_helpers.rb b/app/helpers/authorisation_helpers.rb index 0b25682df6..03b4c970ff 100644 --- a/app/helpers/authorisation_helpers.rb +++ b/app/helpers/authorisation_helpers.rb @@ -31,6 +31,41 @@ def get_permission_hash(role, perm_hash, _other) :get_tutor_times ].freeze + PORTFOLIO_LOCKED_PROJECT_ACTIONS = [ + :make_submission, + :change, + :trigger_week_end, + :reprocess_submission + ].freeze + + PORTFOLIO_LOCKED_TASK_READ_ACTIONS = [ + :get, + :get_submission, + :get_discussion, + :view_plagiarism, + :review_own_attempt, + :review_other_attempt + ].freeze + + def portfolio_lock_project_for(object) + return object if object.is_a?(Project) + return object.project if object.respond_to?(:project) + return object.task.project if object.respond_to?(:task) && object.task.respond_to?(:project) + + nil + end + + def portfolio_lock_blocks?(object, action) + project = portfolio_lock_project_for(object) + return false unless project&.portfolio_locked? + + if object.is_a?(Project) + PORTFOLIO_LOCKED_PROJECT_ACTIONS.include?(action) + else + !PORTFOLIO_LOCKED_TASK_READ_ACTIONS.include?(action) + end + end + # # Authorises if the user can perform an action on the object # @@ -44,6 +79,8 @@ def authorise?(user, object, action, perm_get_fn = method(:get_permission_hash), obj_class = object.class == Class ? object : object.class perm_hash = obj_class.permissions + return false if object.class != Class && portfolio_lock_blocks?(object, action) + # System administrator permissions take precedence over contextual roles. if user.has_admin_capability? system_perms = perm_get_fn.call(user.role.to_sym, perm_hash, other) @@ -75,5 +112,7 @@ def authorise?(user, object, action, perm_get_fn = method(:get_permission_hash), end module_function :get_permission_hash + module_function :portfolio_lock_project_for + module_function :portfolio_lock_blocks? module_function :authorise? end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c74883d014..5a33c789c1 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -28,6 +28,7 @@ class TaskComment < ApplicationRecord validates :recipient, presence: true validates :comment, length: { minimum: 0, maximum: 4095, allow_blank: true } validate :valid_reply_to?, on: :create + validate :prevent_changes_when_portfolio_locked # After create, mark as read by user creating after_create do @@ -35,8 +36,26 @@ class TaskComment < ApplicationRecord end # Delete action - before dependent association + before_destroy :prevent_destroy_when_portfolio_locked before_destroy :delete_associated_files + def prevent_changes_when_portfolio_locked + return unless task&.project&.portfolio_locked? + return if task.portfolio_lock_bypass + + errors.add(:base, 'Comment cannot be changed while the project portfolio is submitted') + end + + def prevent_destroy_when_portfolio_locked + return unless task&.project&.portfolio_locked? + return if task.destroyed? || task.marked_for_destruction? + return if task.project.destroyed? || task.project.marked_for_destruction? + return if task.portfolio_lock_bypass + + errors.add(:base, 'Comment cannot be deleted while the project portfolio is submitted') + throw :abort + end + def valid_reply_to? if reply_to_id.present? originalTaskComment = TaskComment.find(reply_to_id) diff --git a/app/models/group.rb b/app/models/group.rb index fec42947a6..d0f3d5c7ef 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -236,25 +236,33 @@ def create_submission(submitter_task, notes, contributors) gs.group = self gs.notes = notes gs.submitted_by_project = submitter_task.project - gs.save! - contributors.each do |contrib| - project = contrib[:project] - task = project.matching_task submitter_task + GroupSubmission.transaction do + gs.save! - next if task.task_submission_closed? + contributors.each do |contrib| + project = contrib[:project] + task = project.matching_task submitter_task - if contrib[:pct].to_i > 0 - task.group_submission = gs - task.contribution_pct = contrib[:pct] - task.contribution_pts = contrib[:pts] + next if task.task_submission_closed? + + begin + task.portfolio_lock_bypass = true + if contrib[:pct].to_i > 0 + task.group_submission = gs + task.contribution_pct = contrib[:pct] + task.contribution_pts = contrib[:pts] + end + task.save! + ensure + task.portfolio_lock_bypass = false + end end - task.save - end - if old_gs - old_gs.reload - old_gs.destroy! if old_gs.projects.count.zero? + if old_gs + old_gs.reload + old_gs.destroy! if old_gs.projects.count.zero? + end end # ensure that original task is reloaded... update will have effected a different object diff --git a/app/models/group_submission.rb b/app/models/group_submission.rb index 5016aab970..3d0bc50800 100644 --- a/app/models/group_submission.rb +++ b/app/models/group_submission.rb @@ -17,30 +17,41 @@ class GroupSubmission < ApplicationRecord begin FileHelper.delete_group_submission(group_submission) - # also remove evidence from group members - # rubocop:disable Rails/SkipsModelValidations - tasks.where('portfolio_evidence IS NOT NULL').update_all(portfolio_evidence: nil) - # rubocop:enable Rails/SkipsModelValidations + # Also remove evidence from group members through the explicit group + # operation bypass so frozen members cannot be changed by other paths. + tasks.where.not(portfolio_evidence: nil).find_each do |task| + with_portfolio_lock_bypass(task) do + task.update!(portfolio_evidence: nil) + end + end rescue => e logger.error "Failed to delete group submission #{group_submission.id}. Error: #{e.message}" end end def propagate_transition(initial_task, trigger, by_user, quality) - tasks.each do |task| - next if [TaskStatus.complete.id, TaskStatus.feedback_exceeded.id, TaskStatus.fail.id].include? task.task_status_id + Task.transaction do + tasks.each do |task| + next if [TaskStatus.complete.id, TaskStatus.feedback_exceeded.id, TaskStatus.fail.id].include? task.task_status_id + next if task == initial_task - if task != initial_task - task.extensions = initial_task.extensions unless initial_task.extensions < task.extensions - task.trigger_transition(trigger: trigger, by_user: by_user, group_transition: true, quality: quality) + with_portfolio_lock_bypass(task) do + task.extensions = initial_task.extensions unless initial_task.extensions < task.extensions + task.trigger_transition(trigger: trigger, by_user: by_user, group_transition: true, quality: quality) + end end end end def propagate_grade(initial_task, new_grade, ui) - tasks.each do |task| - if task != initial_task - task.grade_task new_grade, ui, grading_group = true + Task.transaction do + tasks.each do |task| + next if task == initial_task + + with_portfolio_lock_bypass(task) do + task.grade_task new_grade, ui, grading_group = true + task.save! + end end end end @@ -63,4 +74,13 @@ def submitted_by? project end delegate :processing_pdf?, to: :submitter_task + + private + + def with_portfolio_lock_bypass(task) + task.portfolio_lock_bypass = true + yield + ensure + task.portfolio_lock_bypass = false + end end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index d01a3210ed..de94c4ba8e 100644 --- a/app/models/overseer_assessment.rb +++ b/app/models/overseer_assessment.rb @@ -274,7 +274,7 @@ def update_from_output(work_dir_path) self.result_task_status = task.status end - if task.ready_for_feedback? && new_status.present? + if task.ready_for_feedback? && new_status.present? && !task.project.portfolio_locked? task.add_status_comment(task.task_definition.unit.main_convenor.user, new_status) task.update task_status: new_status end diff --git a/app/models/pdf_generation/project_compile_portfolio_module.rb b/app/models/pdf_generation/project_compile_portfolio_module.rb index eab5a5292b..b87ea20f3d 100644 --- a/app/models/pdf_generation/project_compile_portfolio_module.rb +++ b/app/models/pdf_generation/project_compile_portfolio_module.rb @@ -146,6 +146,10 @@ def create_portfolio save! true rescue StandardError => e + # A partially written output is not an available portfolio. Removing it + # ensures a failed compilation releases the submission lock. + FileUtils.rm_f(portfolio_path) + self.portfolio_production_date = nil self.compile_portfolio = false save! @@ -262,6 +266,8 @@ def portfolio_tmp_file_path(dict) end def move_to_portfolio(file, name, kind) + raise ActiveRecord::ReadOnlyRecord, 'Portfolio files are frozen after portfolio submission' if portfolio_locked? + # get path to portfolio dir # get path to tmp folder where file parts will be stored portfolio_tmp_dir = portfolio_temp_path @@ -320,6 +326,8 @@ def portfolio_files(ensure_valid: false, force_ascii: false) # Remove a file from the portfolio tmp folder def remove_portfolio_file(idx, kind, name) + raise ActiveRecord::ReadOnlyRecord, 'Portfolio files are frozen after portfolio submission' if portfolio_locked? + # get path to portfolio dir portfolio_tmp_dir = portfolio_temp_path return unless Dir.exist? portfolio_tmp_dir diff --git a/app/models/project.rb b/app/models/project.rb index 19dcf40db6..0cd66417e8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -268,6 +268,25 @@ def active? unit.active end + def portfolio_locked? + unit.lock_project_on_portfolio_submission? && (compile_portfolio? || portfolio_available) + end + + def effective_portfolio_deadline + unit.effective_portfolio_deadline_for(self) + end + + def effective_portfolio_deadline_timezone + return nil if unit.portfolio_deadline.blank? + + unit.portfolio_deadline_timezone_for(self).name + end + + def portfolio_deadline_passed?(at: Time.current) + deadline = effective_portfolio_deadline + deadline.present? && at > deadline + end + # # Get a string representation of the Target Grade # @@ -537,6 +556,8 @@ def self.create_task_stats_from(total_task_counts, project_task_counts, target_g end def revert_overdue_tasks + return if portfolio_locked? + tasks.each do |task| next if task.submission_date.blank? diff --git a/app/models/task.rb b/app/models/task.rb index 3091ef372d..b0391348f2 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -6,7 +6,7 @@ class Task < ApplicationRecord include ApplicationHelper include GradeHelper - attr_accessor :discussion_confirmed_for_transition + attr_accessor :discussion_confirmed_for_transition, :portfolio_lock_bypass # # Permissions around task data @@ -118,6 +118,7 @@ def specific_permission_hash(role, perm_hash, _other) end # Delete action - before dependent association + before_destroy :prevent_destroy_when_portfolio_locked before_destroy :delete_associated_files # Model associations @@ -163,6 +164,7 @@ def specific_permission_hash(role, perm_hash, _other) validate :prevent_complete_if_requires_discussion validate :prevent_feedback_exceeed_if_assess_in_portfolio_enabled validate :prevent_time_exceeed_if_assess_in_portfolio_enabled + validate :prevent_changes_when_portfolio_locked include TaskTiiModule @@ -191,6 +193,17 @@ def prevent_time_exceeed_if_assess_in_portfolio_enabled end end + def prevent_changes_when_portfolio_locked + return unless project&.portfolio_locked? + return if portfolio_lock_bypass + + errors.add(:base, 'Task cannot be changed while the project portfolio is submitted') + end + + def portfolio_lock_prevents_transition?(group_transition) + project.portfolio_locked? && !group_transition && !portfolio_lock_bypass + end + def for_definition_with_quality? task_definition.has_stars? end @@ -632,6 +645,11 @@ def ensured_group_submission def trigger_transition(trigger: '', by_user: nil, bulk: false, group_transition: false, quality: 1, recursive_fix: false, check_feedback: false, system_transition: false) + if portfolio_lock_prevents_transition?(group_transition) + errors.add(:base, 'Project is locked because its portfolio has been submitted') + return nil + end + # # Ensure that assessor is allowed to update the task in the indicated way # @@ -1414,6 +1432,14 @@ def in_process_files_for_task(is_retry) result end + def prevent_destroy_when_portfolio_locked + return unless project&.portfolio_locked? + return if project.destroyed? || project.marked_for_destruction? + + errors.add(:base, 'Task cannot be deleted while the project portfolio is submitted') + throw :abort + end + class TaskAppController < ApplicationController include LatexHelper @@ -1664,6 +1690,8 @@ def stage_word_document_previews(converted_documents) # The student has uploaded new work... # def create_submission_and_trigger_state_change(user, propagate = true, contributions = nil, trigger = 'ready_for_feedback', initial_task = nil) + self.portfolio_lock_bypass = true if group_task? && initial_task.present? + if group_task? && propagate if contributions.nil? # even distribution contribs = group.projects.map { |proj| { project: proj, pct: 100 / group.projects.count, pts: 3 } } @@ -1691,6 +1719,8 @@ def create_submission_and_trigger_state_change(user, propagate = true, contribut save end + ensure + self.portfolio_lock_bypass = false end # diff --git a/app/models/task_definition.rb b/app/models/task_definition.rb index 21789ee38c..a1b6c388e5 100644 --- a/app/models/task_definition.rb +++ b/app/models/task_definition.rb @@ -290,6 +290,8 @@ def update_overdue_tasks_aip overdue_statuses = [TaskStatus.time_exceeded.id] tasks.where(task_status_id: overdue_statuses).find_each do |task| + next if task.project.portfolio_locked? + task.add_status_comment(unit.main_convenor.user, TaskStatus.assess_in_portfolio) task.update(task_status_id: TaskStatus.assess_in_portfolio.id) end @@ -305,6 +307,8 @@ def reset_overdue_tasks .where(task_status: [TaskStatus.time_exceeded, TaskStatus.assess_in_portfolio]) late_submissions.each do |task| + next if task.project.portfolio_locked? + task.add_status_comment(unit.main_convenor.user, TaskStatus.ready_for_feedback) task.update(task_status_id: TaskStatus.ready_for_feedback.id) end diff --git a/app/models/unit.rb b/app/models/unit.rb index 5637e0b039..036a034acf 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -206,6 +206,8 @@ def role_for(user) belongs_to :overseer_image, optional: true + belongs_to :portfolio_deadline_campus, class_name: 'Campus', optional: true + validates :name, :description, :start_date, :end_date, presence: true validates :name, allowed_characters: { type: :unit_name } validates :code, allowed_characters: { type: :unit_code } @@ -243,6 +245,8 @@ def role_for(user) validate :cant_disable_aip_only_if_aip_tasks_exist validate :grade_definitions_are_valid validate :configured_grades_preserve_used_values, if: :will_save_change_to_grade_values? + validate :shared_portfolio_deadline_has_campus + validate :portfolio_deadline_format_is_valid scope :current, -> { current_for_date(Time.zone.now) } scope :current_for_date, ->(date) { where('start_date <= ? AND end_date >= ?', date, date) } @@ -273,6 +277,65 @@ def discuss_timeout_warning_before_expiry errors.add(:discuss_timeout_warning_days, 'must be less than the expiry days') end + def shared_portfolio_deadline_has_campus + return if portfolio_deadline.blank? || portfolio_deadline_per_campus? + return if portfolio_deadline_campus.present? + + errors.add(:portfolio_deadline_campus, 'must be selected when one campus timezone is used for all students') + end + + def portfolio_deadline_format_is_valid + return unless @portfolio_deadline_format_invalid + + errors.add(:portfolio_deadline, 'must use the format YYYY-MM-DDTHH:mm') + end + + # Keep the API terminology while storing the value in the existing column. + def portfolio_deadline + portfolio_due_date + end + + def portfolio_deadline=(value) + @portfolio_deadline_format_invalid = false + if value.is_a?(String) && value.present? + begin + parsed = DateTime.strptime(value, '%Y-%m-%dT%H:%M') + value = Time.zone.local(parsed.year, parsed.month, parsed.day, parsed.hour, parsed.min) + rescue ArgumentError + @portfolio_deadline_format_invalid = true + value = nil + end + end + + self.portfolio_due_date = value + end + + def portfolio_deadline_timezone_for(project) + timezone_name = if portfolio_deadline_per_campus? + project&.campus&.timezone + else + portfolio_deadline_campus&.timezone + end + + ActiveSupport::TimeZone[timezone_name.presence || Time.zone.name] || Time.zone + end + + def effective_portfolio_deadline_for(project) + return nil if portfolio_deadline.blank? + + timezone = portfolio_deadline_timezone_for(project) + local_deadline = timezone.local( + portfolio_deadline.year, + portfolio_deadline.month, + portfolio_deadline.day, + portfolio_deadline.hour, + portfolio_deadline.min, + portfolio_deadline.sec + ) + + local_deadline.advance(days: project&.spec_con_days.to_i) + end + def self.notify_discuss_timeouts! set_active.find_each(&:notify_discuss_timeouts!) end @@ -294,6 +357,7 @@ def discuss_timeout_tasks end def notify_discuss_timeout_for(task, teaching_breaks: nil, now_time: Time.zone.now) + return 0 if task.project.portfolio_locked? return 0 if task.moved_to_discuss_at.blank? actor = task.project.tutor_for(task.task_definition) || main_convenor&.user @@ -636,6 +700,18 @@ def rollover(teaching_period, start_date, end_date, new_code) new_unit.portfolio_auto_generation_date = new_unit.date_for_week_and_day(week_number(self.portfolio_auto_generation_date), Date::ABBR_DAYNAMES[self.portfolio_auto_generation_date.wday]) end + if portfolio_deadline.present? + shifted_deadline = new_unit.date_for_week_and_day( + week_number(portfolio_deadline), + Date::ABBR_DAYNAMES[portfolio_deadline.wday] + ) + new_unit.portfolio_deadline = shifted_deadline.change( + hour: portfolio_deadline.hour, + min: portfolio_deadline.min, + sec: portfolio_deadline.sec + ) + end + # Clear main convenor - do not use old role id new_unit.main_convenor_id = nil @@ -3984,6 +4060,8 @@ def update_overdue_tasks_aip overdue_statuses = [TaskStatus.time_exceeded.id] tasks.where(task_status_id: overdue_statuses).find_each do |task| + next if task.project.portfolio_locked? + task.add_status_comment(main_convenor.user, TaskStatus.assess_in_portfolio) task.update(task_status_id: TaskStatus.assess_in_portfolio.id) end diff --git a/app/sidekiq/accept_overseer_job.rb b/app/sidekiq/accept_overseer_job.rb index 59ddd753fd..ddbe0d7fd9 100644 --- a/app/sidekiq/accept_overseer_job.rb +++ b/app/sidekiq/accept_overseer_job.rb @@ -47,7 +47,7 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment failure_status = nil comment = task.comments.find_by(commentable: oa) - unless comment + unless comment || task.project.portfolio_locked? oa.add_assessment_comment("Tests in progress") end @@ -87,11 +87,11 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment end end - oa.update_assessment_comment("Tests complete: #{steps_passed} / #{active_overseer_steps.count}") + oa.update_assessment_comment("Tests complete: #{steps_passed} / #{active_overseer_steps.count}") unless task.project.portfolio_locked? if steps_attempted == steps_passed && assessment_pass oa.update!(status: :passed) - unless success_status.nil? + unless success_status.nil? || task.project.portfolio_locked? # TODO: have an override status setting for the step? eg. if the task is overdue, let it remain overdue, otherwise use this task status task.update!(task_status: success_status) task.add_status_comment(task.project.tutor_for(task.task_definition), success_status) @@ -102,13 +102,15 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment oa.update!(status: :failed) # preserve_status_on_failure = [TaskStatus.time_exceeded.id, TaskStatus.assess_in_portfolio.id].include?(task.task_status_id) - unless failure_status.nil? # || preserve_status_on_failure + unless failure_status.nil? || task.project.portfolio_locked? # || preserve_status_on_failure # TODO: have an override status setting for the step? eg. if the task is overdue, let it remain overdue, otherwise use this task status task.update!(task_status: failure_status) task.add_status_comment(task.project.tutor_for(task.task_definition), failure_status) oa.update!(result_task_status: failure_status.status_key.to_s) end - task.add_text_comment(task.project.tutor_for(task.task_definition), "**Automated comment**: Some tests did not pass for this submission. Please review the Overseer report, verify your output, and resubmit.") + unless task.project.portfolio_locked? + task.add_text_comment(task.project.tutor_for(task.task_definition), "**Automated comment**: Some tests did not pass for this submission. Please review the Overseer report, verify your output, and resubmit.") + end end FileUtils.rm_rf(work_dir) diff --git a/app/sidekiq/execute_communication_set_job.rb b/app/sidekiq/execute_communication_set_job.rb index 5f99bd3fe6..e748203b26 100644 --- a/app/sidekiq/execute_communication_set_job.rb +++ b/app/sidekiq/execute_communication_set_job.rb @@ -242,6 +242,17 @@ def execute_task_comment_action(action, projects, unit, rule) } end + if project.portfolio_locked? + next { + action_id: action.id, + action_type: action.type, + status: 'skipped', + project_id: project.id, + username: project.user&.username, + reason: 'portfolio submitted' + } + end + task = project.task_for_task_definition(task_definition) rendered_comment = render_template(comment_text_template, project, unit, rule, projects.length) diff --git a/app/sidekiq/refresh_moderation_feedback_timestamps_job.rb b/app/sidekiq/refresh_moderation_feedback_timestamps_job.rb index ad3d8abf77..5239351f9d 100644 --- a/app/sidekiq/refresh_moderation_feedback_timestamps_job.rb +++ b/app/sidekiq/refresh_moderation_feedback_timestamps_job.rb @@ -42,7 +42,7 @@ def perform next if task.last_tutor_feedback_at == latest_feedback_time - task.update!(last_tutor_feedback_at: latest_feedback_time) + task.update!(last_tutor_feedback_at: latest_feedback_time) unless task.project.portfolio_locked? rescue StandardError next end diff --git a/db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb b/db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb new file mode 100644 index 0000000000..0df133ba56 --- /dev/null +++ b/db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb @@ -0,0 +1,7 @@ +class AddPortfolioDeadlineAndSubmissionLock < ActiveRecord::Migration[8.0] + def change + add_column :units, :lock_project_on_portfolio_submission, :boolean, default: false, null: false + add_column :units, :portfolio_deadline_per_campus, :boolean, default: true, null: false + add_reference :units, :portfolio_deadline_campus, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index d37f17db02..8c37dfe90e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_08_12_020302) do +ActiveRecord::Schema[8.0].define(version: 2026_08_17_000000) do create_table "activity_types", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false @@ -951,9 +951,13 @@ t.boolean "discuss_timeout_enabled", default: false, null: false t.integer "discuss_timeout_warning_days", default: 7, null: false t.integer "discuss_timeout_expire_days", default: 14, null: false + t.boolean "lock_project_on_portfolio_submission", default: false, null: false + t.boolean "portfolio_deadline_per_campus", default: true, null: false + t.bigint "portfolio_deadline_campus_id" t.index ["draft_task_definition_id"], name: "index_units_on_draft_task_definition_id" t.index ["main_convenor_id"], name: "index_units_on_main_convenor_id" t.index ["overseer_image_id"], name: "index_units_on_overseer_image_id" + t.index ["portfolio_deadline_campus_id"], name: "index_units_on_portfolio_deadline_campus_id" t.index ["teaching_period_id"], name: "index_units_on_teaching_period_id" t.check_constraint "json_valid(`grade_values`)", name: "grade_values" end diff --git a/test/api/portfolio_submission_lock_api_test.rb b/test/api/portfolio_submission_lock_api_test.rb new file mode 100644 index 0000000000..c7a0344e89 --- /dev/null +++ b/test/api/portfolio_submission_lock_api_test.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PortfolioSubmissionLockApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include ActiveSupport::Testing::TimeHelpers + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def create_portfolio_project(deadline: '2026-08-17T10:00') + campus = FactoryBot.create(:campus, timezone: 'UTC') + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 0, + tutorials: 0, + outcome_count: 0, + staff_count: 0, + campus_count: 0, + lock_project_on_portfolio_submission: true, + portfolio_deadline: deadline, + portfolio_deadline_per_campus: true + ) + project = FactoryBot.create(:project, unit: unit, campus: campus) + FileUtils.touch(project.portfolio_path) + project.update!(portfolio_production_date: Time.zone.now) + project + end + + def delete_portfolio(project, confirm_late: nil) + path = "/api/submission/project/#{project.id}/portfolio" + if confirm_late.nil? + delete path + else + delete path, { confirm_late: confirm_late }.to_json, 'CONTENT_TYPE' => 'application/json' + end + end + + def test_student_must_confirm_deletion_after_effective_deadline + project = create_portfolio_project + + travel_to(Time.zone.parse('2026-08-17 10:00:01 UTC')) do + add_auth_header_for(user: project.student) + delete_portfolio(project) + + assert_equal 409, last_response.status + assert last_response_body['portfolio_late_confirmation_required'] + assert_equal '2026-08-17T10:00:00Z', last_response_body['effective_portfolio_deadline'] + assert_equal 'UTC', last_response_body['effective_portfolio_deadline_timezone'] + assert project.reload.portfolio_locked? + + delete_portfolio(project, confirm_late: true) + assert_includes [200, 204], last_response.status + end + + assert_not project.reload.portfolio_locked? + assert_not File.exist?(project.portfolio_path) + assert File.exist?("#{project.portfolio_path}.old") + end + + def test_exact_deadline_does_not_require_late_confirmation + project = create_portfolio_project + + travel_to(Time.zone.parse('2026-08-17 10:00:00 UTC')) do + add_auth_header_for(user: project.student) + delete_portfolio(project) + assert_includes [200, 204], last_response.status + end + end + + def test_compiling_portfolio_cannot_be_deleted + project = create_portfolio_project(deadline: nil) + FileUtils.rm_f(project.portfolio_path) + project.update!(compile_portfolio: true, portfolio_production_date: nil) + add_auth_header_for(user: project.student) + + delete_portfolio(project) + + assert_equal 409, last_response.status + assert_match(/still compiling/, last_response_body['error']) + assert project.reload.portfolio_locked? + end + + def test_tutor_cannot_delete_a_portfolio + project = create_portfolio_project(deadline: nil) + tutor = FactoryBot.create(:user, :tutor) + project.unit.employ_staff(tutor, Role.tutor) + add_auth_header_for(user: tutor) + + delete_portfolio(project) + + assert_equal 403, last_response.status + assert project.reload.portfolio_locked? + end + + def test_convenor_can_delete_a_portfolio + project = create_portfolio_project(deadline: nil) + convenor = FactoryBot.create(:user, :convenor) + project.unit.employ_staff(convenor, Role.convenor) + add_auth_header_for(user: convenor) + + delete_portfolio(project) + + assert_includes [200, 204], last_response.status + assert_not project.reload.portfolio_locked? + end + + def test_convenor_can_configure_and_read_portfolio_submission_settings + project = create_portfolio_project(deadline: nil) + unit = project.unit + timezone_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth') + add_auth_header_for(user: unit.main_convenor_user) + + put_json( + "/api/units/#{unit.id}", + unit: { + portfolio_deadline: '2026-09-12T17:45', + portfolio_deadline_per_campus: false, + portfolio_deadline_campus_id: timezone_campus.id, + lock_project_on_portfolio_submission: true + } + ) + + assert_equal 200, last_response.status, last_response_body + get "/api/units/#{unit.id}" + assert_equal 200, last_response.status, last_response_body + assert_equal '2026-09-12T17:45', last_response_body['portfolio_deadline'] + assert_not last_response_body['portfolio_deadline_per_campus'] + assert_equal timezone_campus.id, last_response_body['portfolio_deadline_campus_id'] + assert last_response_body['lock_project_on_portfolio_submission'] + end +end diff --git a/test/api/projects_api_test.rb b/test/api/projects_api_test.rb index b6a41ccc75..661b18d834 100644 --- a/test/api/projects_api_test.rb +++ b/test/api/projects_api_test.rb @@ -52,7 +52,7 @@ def test_projects_returns_correct_data # Add username and auth_token to Header add_auth_header_for(user: user) - keys = %w[id unit campus_id user_id target_grade portfolio_available spec_con_days escalation_attempts_remaining] + keys = %w[id unit campus_id user_id target_grade portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed spec_con_days escalation_attempts_remaining] key_test = %w[campus_id target_grade spec_con_days] get '/api/projects' @@ -77,8 +77,8 @@ def test_get_project_response_is_correct # Add username and auth_token to Header add_auth_header_for(user: user) - keys = %w[id unit unit_id user_id campus_id target_grade submitted_grade portfolio_files compile_portfolio portfolio_available uses_draft_learning_summary tasks tutorial_enrolments groups spec_con_days escalation_attempts_remaining] - key_test = keys - %w[unit user_id portfolio_available tasks tutorial_enrolments groups] + keys = %w[id unit unit_id user_id campus_id target_grade submitted_grade portfolio_files compile_portfolio portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed uses_draft_learning_summary tasks tutorial_enrolments groups spec_con_days escalation_attempts_remaining] + key_test = keys - %w[unit user_id portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed tasks tutorial_enrolments groups] get "/api/projects/#{project.id}" assert_equal 200, last_response.status, last_response_body @@ -227,10 +227,15 @@ def test_submitted_grade_cant_change_after_submission assert_equal 200, last_response.status, last_response_body assert_equal user.projects.find(project.id).submitted_grade, 2 - keys = %w(campus_id target_grade submitted_grade compile_portfolio portfolio_available uses_draft_learning_summary) + keys = %w[ + campus_id target_grade submitted_grade compile_portfolio portfolio_available portfolio_locked + effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed + uses_draft_learning_summary + ] assert_json_limit_keys_to_exactly keys, last_response_body - assert_json_matches_model project, last_response_body, keys + assert_json_matches_model project, last_response_body, + keys - %w[portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed] DatabasePopulator.generate_portfolio(project) diff --git a/test/api/units_api_test.rb b/test/api/units_api_test.rb index 8888ba3ad5..94903532e9 100644 --- a/test/api/units_api_test.rb +++ b/test/api/units_api_test.rb @@ -325,7 +325,13 @@ def test_unit_output assert_equal actual_unit['start_date'].to_date, expected_unit.start_date.to_date assert_equal actual_unit['end_date'].to_date, expected_unit.end_date.to_date - keys = %w[code id name main_convenor_id description active auto_apply_extension_before_deadline send_notifications enable_sync_enrolments enable_sync_timetable draft_task_definition_id allow_student_extension_requests extension_weeks_on_resubmit_request allow_student_change_tutorial] + keys = %w[ + code id name main_convenor_id description active auto_apply_extension_before_deadline + send_notifications enable_sync_enrolments enable_sync_timetable draft_task_definition_id + allow_student_extension_requests extension_weeks_on_resubmit_request + allow_student_change_tutorial portfolio_deadline portfolio_deadline_per_campus + portfolio_deadline_campus_id lock_project_on_portfolio_submission + ] assert actual_unit.key?("my_role"), actual_unit.inspect assert_equal expected_unit.role_for(expected_unit.main_convenor_user).name, actual_unit["my_role"] diff --git a/test/models/portfolio_submission_lock_test.rb b/test/models/portfolio_submission_lock_test.rb new file mode 100644 index 0000000000..bb11be314f --- /dev/null +++ b/test/models/portfolio_submission_lock_test.rb @@ -0,0 +1,188 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PortfolioSubmissionLockTest < ActiveSupport::TestCase + def build_unit(**attributes) + FactoryBot.create( + :unit, + { with_students: false, task_count: 0, tutorials: 0, outcome_count: 0, + staff_count: 0, campus_count: 0 }.merge(attributes) + ) + end + + def test_portfolio_settings_default_to_existing_unlocked_behaviour + unit = build_unit + + assert_nil unit.portfolio_deadline + assert unit.portfolio_deadline_per_campus? + assert_not unit.lock_project_on_portfolio_submission? + end + + def test_shared_deadline_requires_a_campus + unit = build_unit + unit.portfolio_deadline = '2026-10-04T23:30' + unit.portfolio_deadline_per_campus = false + + assert_not unit.valid? + assert_includes unit.errors[:portfolio_deadline_campus], + 'must be selected when one campus timezone is used for all students' + + unit.portfolio_deadline_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth') + assert unit.valid?, unit.errors.full_messages.to_sentence + end + + def test_deadline_rejects_an_invalid_local_datetime + unit = build_unit + + unit.portfolio_deadline = '17 August 2026 at five' + + assert_not unit.valid? + assert_includes unit.errors[:portfolio_deadline], 'must use the format YYYY-MM-DDTHH:mm' + end + + def test_effective_deadline_uses_project_campus_and_special_consideration_calendar_days + campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') + unit = build_unit( + portfolio_deadline: '2026-10-03T23:30', + portfolio_deadline_per_campus: true + ) + project = FactoryBot.create(:project, unit: unit, campus: campus, spec_con_days: 2) + + deadline = project.effective_portfolio_deadline + + assert_equal 'Australia/Melbourne', project.effective_portfolio_deadline_timezone + assert_equal '2026-10-05T23:30:00+11:00', deadline.iso8601 + assert_not project.portfolio_deadline_passed?(at: deadline) + assert project.portfolio_deadline_passed?(at: deadline + 1.second) + end + + def test_shared_deadline_uses_selected_campus_even_when_it_is_inactive + project_campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') + deadline_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth', active: false) + unit = build_unit( + portfolio_deadline: '2026-08-17T16:00', + portfolio_deadline_per_campus: false, + portfolio_deadline_campus: deadline_campus + ) + project = FactoryBot.create(:project, unit: unit, campus: project_campus) + + assert_equal 'Australia/Perth', project.effective_portfolio_deadline_timezone + assert_equal '2026-08-17T16:00:00+08:00', project.effective_portfolio_deadline.iso8601 + end + + def test_deadline_without_a_project_campus_uses_the_deployment_timezone + unit = build_unit( + portfolio_deadline: '2026-08-17T16:00', + portfolio_deadline_per_campus: true + ) + project = FactoryBot.create(:project, unit: unit, campus: nil) + + deadline = project.effective_portfolio_deadline + + assert_equal Time.zone.name, project.effective_portfolio_deadline_timezone + assert_equal [2026, 8, 17, 16, 0], + [deadline.year, deadline.month, deadline.day, deadline.hour, deadline.min] + end + + def test_lock_is_immediate_retroactive_and_clears_after_compile_failure + unit = build_unit(lock_project_on_portfolio_submission: false) + project = FactoryBot.create(:project, unit: unit, compile_portfolio: true) + + assert_not project.portfolio_locked? + + unit.update!(lock_project_on_portfolio_submission: true) + assert project.reload.portfolio_locked? + + project.update!(compile_portfolio: false) + assert_not project.reload.portfolio_locked? + end + + def test_locked_project_rejects_task_and_comment_changes_and_destruction + unit = build_unit(lock_project_on_portfolio_submission: true) + definition = FactoryBot.create(:task_definition, unit: unit) + project = FactoryBot.create(:project, unit: unit) + task = FactoryBot.create(:task, project: project, task_definition: definition) + comment = TaskComment.create!( + task: task, + user: project.student, + recipient: project.student, + comment: 'Submitted evidence' + ) + + project.update!(compile_portfolio: true) + + task.task_status = TaskStatus.ready_for_feedback + assert_not task.save + assert_includes task.errors[:base], 'Task cannot be changed while the project portfolio is submitted' + + comment.comment = 'Changed evidence' + assert_not comment.save + assert_not comment.destroy + assert Task.exists?(task.id) + assert TaskComment.exists?(comment.id) + assert_not task.destroy + assert Task.exists?(task.id) + + assert_not definition.destroy + assert TaskDefinition.exists?(definition.id) + assert Task.exists?(task.id) + end + + def test_group_operations_can_use_the_explicit_bypass + unit = build_unit(lock_project_on_portfolio_submission: true) + definition = FactoryBot.create(:task_definition, unit: unit) + project = FactoryBot.create(:project, unit: unit, compile_portfolio: true) + task = FactoryBot.build(:task, project: project, task_definition: definition) + + assert_not task.save + + task.portfolio_lock_bypass = true + assert task.save, task.errors.full_messages.to_sentence + end + + def test_group_submission_updates_a_frozen_member_transactionally + unit = FactoryBot.create( + :unit, + lock_project_on_portfolio_submission: true, + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 1, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + group_tasks: [{ idx: 0, gs: 0 }] + ) + group = unit.groups.first + submitter_project, frozen_project = group.projects.to_a + submitter_task = submitter_project.task_for_task_definition(unit.task_definitions.first) + frozen_task = frozen_project.task_for_task_definition(unit.task_definitions.first) + frozen_project.update!(compile_portfolio: true) + contributions = group.projects.map { |project| { project: project, pct: 50, pts: 3 } } + + submission = group.create_submission(submitter_task, 'Group submission', contributions) + + assert_equal submission, frozen_task.reload.group_submission + assert_equal 50, frozen_task.contribution_pct + assert frozen_project.reload.portfolio_locked? + end + + def test_lock_authorisation_applies_to_students_tutors_and_convenors_but_not_portfolio_assessment + unit = build_unit(lock_project_on_portfolio_submission: true) + definition = FactoryBot.create(:task_definition, unit: unit) + project = FactoryBot.create(:project, unit: unit) + task = FactoryBot.create(:task, project: project, task_definition: definition) + tutor = FactoryBot.create(:user, :tutor) + convenor = FactoryBot.create(:user, :convenor) + unit.employ_staff(tutor, Role.tutor) + unit.employ_staff(convenor, Role.convenor) + project.update!(compile_portfolio: true) + + assert_not AuthorisationHelpers.authorise?(project.student, task, :make_submission) + assert_not AuthorisationHelpers.authorise?(tutor, task, :make_submission) + assert_not AuthorisationHelpers.authorise?(convenor, task, :make_submission) + assert_not AuthorisationHelpers.authorise?(project.student, project, :change) + assert AuthorisationHelpers.authorise?(convenor, project, :assess) + end +end diff --git a/test/models/unit_model_test.rb b/test/models/unit_model_test.rb index 151415ff84..2ceb052faa 100644 --- a/test/models/unit_model_test.rb +++ b/test/models/unit_model_test.rb @@ -217,6 +217,29 @@ def test_rollover_of_portfolio_generation unit2.destroy end + def test_rollover_retains_and_shifts_portfolio_submission_settings + @unit.update!(portfolio_auto_generation_date: nil) + original_deadline = (@unit.start_date + 3.weeks + 2.days).change(hour: 14, min: 35) + deadline_campus = Campus.first + @unit.update!( + portfolio_deadline: original_deadline.strftime('%Y-%m-%dT%H:%M'), + portfolio_deadline_per_campus: false, + portfolio_deadline_campus: deadline_campus, + lock_project_on_portfolio_submission: true + ) + + unit2 = @unit.rollover TeachingPeriod.find(2), nil, nil, nil + + assert unit2.lock_project_on_portfolio_submission? + refute unit2.portfolio_deadline_per_campus? + assert_equal deadline_campus, unit2.portfolio_deadline_campus + assert_equal @unit.week_number(original_deadline), unit2.week_number(unit2.portfolio_deadline) + assert_equal original_deadline.wday, unit2.portfolio_deadline.wday + assert_equal [14, 35], [unit2.portfolio_deadline.hour, unit2.portfolio_deadline.min] + + unit2.destroy + end + def test_rollover_of_group_tasks unit = FactoryBot.create(:unit, code: 'SIT102', diff --git a/test/sidekiq/execute_communication_set_job_test.rb b/test/sidekiq/execute_communication_set_job_test.rb index 8ec1afb8b3..e93bea9a1a 100644 --- a/test/sidekiq/execute_communication_set_job_test.rb +++ b/test/sidekiq/execute_communication_set_job_test.rb @@ -3,6 +3,38 @@ require 'test_helper' class ExecuteCommunicationSetJobTest < ActiveSupport::TestCase + def test_task_comment_action_skips_a_frozen_project_without_creating_a_task + unit = FactoryBot.create( + :unit, + with_students: false, + task_count: 1, + stream_count: 0, + tutorials: 0, + outcome_count: 0, + staff_count: 1, + lock_project_on_portfolio_submission: true + ) + task_definition = unit.task_definitions.first + project = unit.enrol_student(FactoryBot.create(:user, :student), Campus.first) + project.update!(compile_portfolio: true) + communication_set = unit.communication_sets.create!(name: 'Frozen Set', active: true) + communication_rule = communication_set.communication_rules.create!( + name: 'Frozen Comment Rule', + operator: 'and', + position: 0 + ) + communication_rule.communication_actions.create!( + type: 'TaskCommentAction', + task_definition: task_definition, + body: 'This must not change frozen evidence' + ) + + ExecuteCommunicationSetJob.new.perform(communication_set.id) + + assert_not project.tasks.exists?(task_definition: task_definition) + assert_empty TaskComment.joins(task: :project).where(projects: { id: project.id }) + end + def test_task_comment_action_adds_a_comment_to_each_selected_students_task unit = FactoryBot.create( :unit, @@ -51,7 +83,7 @@ def test_task_comment_action_adds_a_comment_to_each_selected_students_task assert_equal comment_author, comment_one.user assert_equal comment_author, comment_two.user - assert_equal 'Please review Ada for ' + unit.code, comment_one.comment - assert_equal 'Please review Grace for ' + unit.code, comment_two.comment + assert_equal "Please review Ada for #{unit.code}", comment_one.comment + assert_equal "Please review Grace for #{unit.code}", comment_two.comment end end From 822c0126c8cbdd0829bafd1341417395044ed182 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:32:02 +1000 Subject: [PATCH 2/3] chore: bump migration --- ...0260818003147_add_portfolio_deadline_and_submission_lock.rb} | 0 db/schema.rb | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename db/migrate/{20260817000000_add_portfolio_deadline_and_submission_lock.rb => 20260818003147_add_portfolio_deadline_and_submission_lock.rb} (100%) diff --git a/db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb b/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb similarity index 100% rename from db/migrate/20260817000000_add_portfolio_deadline_and_submission_lock.rb rename to db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb diff --git a/db/schema.rb b/db/schema.rb index c0f392bb5a..a47cf5b009 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_08_17_000000) do +ActiveRecord::Schema[8.0].define(version: 2026_08_18_003147) do create_table "activity_types", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false From 089ce32555bd8ed8b5cfc0bde057f14bd5b70946 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:13:17 +1000 Subject: [PATCH 3/3] refactor: minify scope down to portfolio lock only --- app/api/entities/project_entity.rb | 7 -- app/api/entities/unit_entity.rb | 14 +-- app/api/projects_api.rb | 3 - app/api/submission/portfolio_api.rb | 35 +++--- app/api/task_comments_api.rb | 3 +- app/api/units_api.rb | 16 +-- app/helpers/authorisation_helpers.rb | 39 ------- app/models/comments/task_comment.rb | 2 - app/models/group.rb | 38 +++--- app/models/group_submission.rb | 46 +++----- .../project_compile_portfolio_module.rb | 4 - app/models/project.rb | 18 +-- app/models/task.rb | 21 ++-- app/models/unit.rb | 75 ------------ ..._portfolio_deadline_and_submission_lock.rb | 2 - db/schema.rb | 3 - .../api/portfolio_submission_lock_api_test.rb | 95 ++++----------- test/api/projects_api_test.rb | 15 +-- test/api/units_api_test.rb | 3 +- test/models/portfolio_submission_lock_test.rb | 109 +----------------- test/models/unit_model_test.rb | 23 ---- .../execute_communication_set_job_test.rb | 4 +- 22 files changed, 99 insertions(+), 476 deletions(-) diff --git a/app/api/entities/project_entity.rb b/app/api/entities/project_entity.rb index f75e14b01d..f760249c0a 100644 --- a/app/api/entities/project_entity.rb +++ b/app/api/entities/project_entity.rb @@ -21,13 +21,6 @@ class ProjectEntity < Grape::Entity expose :portfolio_locked do |project| project.portfolio_locked? end - expose :effective_portfolio_deadline, expose_nil: true do |project| - project.effective_portfolio_deadline&.iso8601 - end - expose :effective_portfolio_deadline_timezone, expose_nil: true - expose :portfolio_deadline_passed do |project| - project.portfolio_deadline_passed? - end # rubocop:enable Style/SymbolProc expose :portfolio_submission_date, if: :for_staff expose :uses_draft_learning_summary, unless: :summary_only diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index cce564d6b1..f73f757ae5 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -6,10 +6,6 @@ class UnitEntity < Grape::Entity date.strftime('%Y-%m-%d') end - format_with(:local_datetime) do |date| - date&.strftime('%Y-%m-%dT%H:%M') - end - def is_staff?(my_role) [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? end @@ -66,13 +62,9 @@ def can_read_unit_config?(my_role) expose :allow_student_change_tutorial, unless: :summary_only expose :allow_flexible_dates, unless: :summary_only expose :mark_late_submissions_as_assess_in_portfolio, unless: :summary_only - - with_options(unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }) do - expose :portfolio_deadline, format_with: :local_datetime, expose_nil: true - expose :portfolio_deadline_per_campus - expose :portfolio_deadline_campus_id, expose_nil: true - expose :lock_project_on_portfolio_submission - end + # Students need to see this too, so the portfolio review step can warn them + # that submitting will lock their tasks - it is not staff-only. + expose :lock_project_on_portfolio_submission, unless: :summary_only expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only diff --git a/app/api/projects_api.rb b/app/api/projects_api.rb index 36f552b1b0..971a929a22 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -175,9 +175,6 @@ class ProjectsApi < Grape::API :compile_portfolio, :portfolio_available, :portfolio_locked, - :effective_portfolio_deadline, - :effective_portfolio_deadline_timezone, - :portfolio_deadline_passed, :uses_draft_learning_summary, :stats ], diff --git a/app/api/submission/portfolio_api.rb b/app/api/submission/portfolio_api.rb index 1e934d2a7e..6188571763 100644 --- a/app/api/submission/portfolio_api.rb +++ b/app/api/submission/portfolio_api.rb @@ -24,6 +24,10 @@ class PortfolioApi < Grape::API error!({ error: "Not authorised to submit portfolio for project '#{params[:id]}'" }, 401) end + if project.portfolio_locked? + error!({ error: 'Portfolio evidence is frozen because the portfolio has already been submitted.' }, 403) + end + file = params[:file0] name = params[:name] kind = params[:kind] @@ -45,30 +49,19 @@ class PortfolioApi < Grape::API optional :idx, type: Integer, desc: 'The index of the file' optional :kind, type: String, desc: 'The kind of file being removed: document, code, or image' optional :name, type: String, desc: 'Name of file to remove' - optional :confirm_late, type: Boolean, default: false, desc: 'Confirm deletion after the effective portfolio deadline' end delete '/submission/project/:id/portfolio' do project = Project.find(params[:id]) - deleting_whole_portfolio = params[:idx].nil? && params[:name].nil? && params[:kind].nil? - if deleting_whole_portfolio - unit_role = project.unit.unit_role_for(current_user) - can_delete = project.user_id == current_user.id || unit_role&.role_id == Role.convenor_id - unless can_delete - error!({ error: "Not authorised to delete portfolio for project '#{params[:id]}'" }, 403) - end - if project.compile_portfolio? - error!({ error: 'The portfolio is still compiling. Wait for compilation to finish before deleting it.' }, 409) - end - if project.portfolio_deadline_passed? && !params[:confirm_late] - error!({ - error: 'Deleting this portfolio requires confirmation because the effective portfolio deadline has passed.', - portfolio_late_confirmation_required: true, - effective_portfolio_deadline: project.effective_portfolio_deadline.iso8601, - effective_portfolio_deadline_timezone: project.effective_portfolio_deadline_timezone - }, 409) - end + unless authorise? current_user, project, :make_submission + error!({ error: "Not authorised to alter portfolio for project '#{params[:id]}'" }, 401) + end + # Remove file or portfolio? + if params[:idx].nil? && params[:name].nil? && params[:kind].nil? + # Deleting the whole portfolio is how a locked project gets unlocked again, + # so it is intentionally still allowed once a portfolio has been submitted. + # compile_portfolio is cleared too so a delete performed mid-compile still unlocks the project. project.update!({ portfolio_submission_date: nil, portfolio_production_date: nil, @@ -76,8 +69,8 @@ class PortfolioApi < Grape::API }) project.remove_portfolio # returns details of file elsif !(params[:idx].nil? || params[:name].nil? || params[:kind].nil?) - unless authorise? current_user, project, :make_submission - error!({ error: "Not authorised to alter portfolio for project '#{params[:id]}'" }, 403) + if project.portfolio_locked? + error!({ error: 'Portfolio evidence is frozen because the portfolio has already been submitted.' }, 403) end idx = params[:idx] diff --git a/app/api/task_comments_api.rb b/app/api/task_comments_api.rb index 75d843224a..cfe2500a87 100644 --- a/app/api/task_comments_api.rb +++ b/app/api/task_comments_api.rb @@ -259,8 +259,7 @@ class TaskCommentsApi < Grape::API project = Project.find(params[:project_id]) task_definition = project.unit.task_definitions.find(params[:task_definition_id]) - # Read-receipt state remains editable even when portfolio evidence is frozen. - unless authorise? current_user, project, :get + unless authorise? current_user, project, :make_submission error!({ error: 'Not authorised to mark comment as unread' }, 403) end diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 2eefe817d6..7567be0bc3 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -85,10 +85,7 @@ class UnitsApi < Grape::API optional :enable_sync_enrolments, type: Boolean, desc: 'Sync student enrolments automatically if supported by deployment' optional :draft_task_definition_id, type: Integer, desc: 'Indicates the ID of the task definition used as the "draft learning summary task"' optional :portfolio_auto_generation_date, type: Date, desc: 'Indicates a date where student portfolio will automatically compile' - optional :portfolio_deadline, type: String, desc: 'Portfolio deadline entered as YYYY-MM-DDTHH:mm local time' - optional :portfolio_deadline_per_campus, type: Boolean, desc: 'Apply the local deadline in each student campus timezone' - optional :portfolio_deadline_campus_id, type: Integer, desc: 'Campus timezone used for all students when not applying per campus' - optional :lock_project_on_portfolio_submission, type: Boolean, desc: 'Freeze portfolio evidence while a portfolio is compiling or available' + optional :lock_project_on_portfolio_submission, type: Boolean, desc: 'Freeze task statuses, comments, and portfolio evidence while a portfolio is compiling or available' optional :allow_flexible_dates, type: Boolean, desc: 'Can turn on/off flexible dates for tasks in this unit' optional :allow_student_extension_requests, type: Boolean, desc: 'Can turn on/off student extension requests' optional :allow_student_change_tutorial, type: Boolean, desc: 'Can turn on/off student ability to change tutorials' @@ -134,9 +131,6 @@ class UnitsApi < Grape::API :enable_sync_enrolments, :draft_task_definition_id, :portfolio_auto_generation_date, - :portfolio_deadline, - :portfolio_deadline_per_campus, - :portfolio_deadline_campus_id, :lock_project_on_portfolio_submission, :allow_flexible_dates, :allow_student_extension_requests, @@ -195,10 +189,7 @@ class UnitsApi < Grape::API optional :allow_student_extension_requests, type: Boolean, desc: 'Can turn on/off student extension requests', default: true optional :extension_weeks_on_resubmit_request, type: Integer, desc: 'Determines the number of weeks extension on a resubmit request', default: 1 optional :portfolio_auto_generation_date, type: Date, desc: 'Indicates a date where student portfolio will automatically compile' - optional :portfolio_deadline, type: String, desc: 'Portfolio deadline entered as YYYY-MM-DDTHH:mm local time' - optional :portfolio_deadline_per_campus, type: Boolean, default: true - optional :portfolio_deadline_campus_id, type: Integer - optional :lock_project_on_portfolio_submission, type: Boolean, default: false + optional :lock_project_on_portfolio_submission, type: Boolean, desc: 'Freeze task statuses, comments, and portfolio evidence while a portfolio is compiling or available', default: false optional :allow_student_change_tutorial, type: Boolean, desc: 'Can turn on/off student ability to change tutorials', default: true optional :feedback_warning_threshold_days, type: Integer, desc: 'Number of days since a submission without feedback before its highlighted in the tutors inbox' optional :feedback_overflow_threshold_days, type: Integer, desc: 'Number of days since a submission without feedback before its added to overflow marking' @@ -241,9 +232,6 @@ class UnitsApi < Grape::API :allow_student_extension_requests, :extension_weeks_on_resubmit_request, :portfolio_auto_generation_date, - :portfolio_deadline, - :portfolio_deadline_per_campus, - :portfolio_deadline_campus_id, :lock_project_on_portfolio_submission, :allow_student_change_tutorial, :feedback_warning_threshold_days, diff --git a/app/helpers/authorisation_helpers.rb b/app/helpers/authorisation_helpers.rb index 03b4c970ff..0b25682df6 100644 --- a/app/helpers/authorisation_helpers.rb +++ b/app/helpers/authorisation_helpers.rb @@ -31,41 +31,6 @@ def get_permission_hash(role, perm_hash, _other) :get_tutor_times ].freeze - PORTFOLIO_LOCKED_PROJECT_ACTIONS = [ - :make_submission, - :change, - :trigger_week_end, - :reprocess_submission - ].freeze - - PORTFOLIO_LOCKED_TASK_READ_ACTIONS = [ - :get, - :get_submission, - :get_discussion, - :view_plagiarism, - :review_own_attempt, - :review_other_attempt - ].freeze - - def portfolio_lock_project_for(object) - return object if object.is_a?(Project) - return object.project if object.respond_to?(:project) - return object.task.project if object.respond_to?(:task) && object.task.respond_to?(:project) - - nil - end - - def portfolio_lock_blocks?(object, action) - project = portfolio_lock_project_for(object) - return false unless project&.portfolio_locked? - - if object.is_a?(Project) - PORTFOLIO_LOCKED_PROJECT_ACTIONS.include?(action) - else - !PORTFOLIO_LOCKED_TASK_READ_ACTIONS.include?(action) - end - end - # # Authorises if the user can perform an action on the object # @@ -79,8 +44,6 @@ def authorise?(user, object, action, perm_get_fn = method(:get_permission_hash), obj_class = object.class == Class ? object : object.class perm_hash = obj_class.permissions - return false if object.class != Class && portfolio_lock_blocks?(object, action) - # System administrator permissions take precedence over contextual roles. if user.has_admin_capability? system_perms = perm_get_fn.call(user.role.to_sym, perm_hash, other) @@ -112,7 +75,5 @@ def authorise?(user, object, action, perm_get_fn = method(:get_permission_hash), end module_function :get_permission_hash - module_function :portfolio_lock_project_for - module_function :portfolio_lock_blocks? module_function :authorise? end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index 5a33c789c1..5c4558c059 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -41,7 +41,6 @@ class TaskComment < ApplicationRecord def prevent_changes_when_portfolio_locked return unless task&.project&.portfolio_locked? - return if task.portfolio_lock_bypass errors.add(:base, 'Comment cannot be changed while the project portfolio is submitted') end @@ -50,7 +49,6 @@ def prevent_destroy_when_portfolio_locked return unless task&.project&.portfolio_locked? return if task.destroyed? || task.marked_for_destruction? return if task.project.destroyed? || task.project.marked_for_destruction? - return if task.portfolio_lock_bypass errors.add(:base, 'Comment cannot be deleted while the project portfolio is submitted') throw :abort diff --git a/app/models/group.rb b/app/models/group.rb index d0f3d5c7ef..f13451c261 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -236,33 +236,27 @@ def create_submission(submitter_task, notes, contributors) gs.group = self gs.notes = notes gs.submitted_by_project = submitter_task.project + gs.save! - GroupSubmission.transaction do - gs.save! - - contributors.each do |contrib| - project = contrib[:project] - task = project.matching_task submitter_task + contributors.each do |contrib| + project = contrib[:project] + task = project.matching_task submitter_task - next if task.task_submission_closed? + # A group member whose own portfolio is locked keeps the exact task + # state they had when they submitted, so leave their task alone. + next if task.task_submission_closed? || task.project.portfolio_locked? - begin - task.portfolio_lock_bypass = true - if contrib[:pct].to_i > 0 - task.group_submission = gs - task.contribution_pct = contrib[:pct] - task.contribution_pts = contrib[:pts] - end - task.save! - ensure - task.portfolio_lock_bypass = false - end + if contrib[:pct].to_i > 0 + task.group_submission = gs + task.contribution_pct = contrib[:pct] + task.contribution_pts = contrib[:pts] end + task.save + end - if old_gs - old_gs.reload - old_gs.destroy! if old_gs.projects.count.zero? - end + if old_gs + old_gs.reload + old_gs.destroy! if old_gs.projects.count.zero? end # ensure that original task is reloaded... update will have effected a different object diff --git a/app/models/group_submission.rb b/app/models/group_submission.rb index 3d0bc50800..6e04bc7cb4 100644 --- a/app/models/group_submission.rb +++ b/app/models/group_submission.rb @@ -17,12 +17,12 @@ class GroupSubmission < ApplicationRecord begin FileHelper.delete_group_submission(group_submission) - # Also remove evidence from group members through the explicit group - # operation bypass so frozen members cannot be changed by other paths. - tasks.where.not(portfolio_evidence: nil).find_each do |task| - with_portfolio_lock_bypass(task) do - task.update!(portfolio_evidence: nil) - end + # also remove evidence from group members - skip anyone whose portfolio + # is locked so their task state stays exactly as submitted + tasks.where('portfolio_evidence IS NOT NULL').find_each do |task| + next if task.project.portfolio_locked? + + task.update(portfolio_evidence: nil) end rescue => e logger.error "Failed to delete group submission #{group_submission.id}. Error: #{e.message}" @@ -30,28 +30,23 @@ class GroupSubmission < ApplicationRecord end def propagate_transition(initial_task, trigger, by_user, quality) - Task.transaction do - tasks.each do |task| - next if [TaskStatus.complete.id, TaskStatus.feedback_exceeded.id, TaskStatus.fail.id].include? task.task_status_id - next if task == initial_task + tasks.each do |task| + next if [TaskStatus.complete.id, TaskStatus.feedback_exceeded.id, TaskStatus.fail.id].include? task.task_status_id + next if task.project.portfolio_locked? - with_portfolio_lock_bypass(task) do - task.extensions = initial_task.extensions unless initial_task.extensions < task.extensions - task.trigger_transition(trigger: trigger, by_user: by_user, group_transition: true, quality: quality) - end + if task != initial_task + task.extensions = initial_task.extensions unless initial_task.extensions < task.extensions + task.trigger_transition(trigger: trigger, by_user: by_user, group_transition: true, quality: quality) end end end def propagate_grade(initial_task, new_grade, ui) - Task.transaction do - tasks.each do |task| - next if task == initial_task + tasks.each do |task| + next if task.project.portfolio_locked? - with_portfolio_lock_bypass(task) do - task.grade_task new_grade, ui, grading_group = true - task.save! - end + if task != initial_task + task.grade_task new_grade, ui, grading_group = true end end end @@ -74,13 +69,4 @@ def submitted_by? project end delegate :processing_pdf?, to: :submitter_task - - private - - def with_portfolio_lock_bypass(task) - task.portfolio_lock_bypass = true - yield - ensure - task.portfolio_lock_bypass = false - end end diff --git a/app/models/pdf_generation/project_compile_portfolio_module.rb b/app/models/pdf_generation/project_compile_portfolio_module.rb index b87ea20f3d..7448ee1fc5 100644 --- a/app/models/pdf_generation/project_compile_portfolio_module.rb +++ b/app/models/pdf_generation/project_compile_portfolio_module.rb @@ -266,8 +266,6 @@ def portfolio_tmp_file_path(dict) end def move_to_portfolio(file, name, kind) - raise ActiveRecord::ReadOnlyRecord, 'Portfolio files are frozen after portfolio submission' if portfolio_locked? - # get path to portfolio dir # get path to tmp folder where file parts will be stored portfolio_tmp_dir = portfolio_temp_path @@ -326,8 +324,6 @@ def portfolio_files(ensure_valid: false, force_ascii: false) # Remove a file from the portfolio tmp folder def remove_portfolio_file(idx, kind, name) - raise ActiveRecord::ReadOnlyRecord, 'Portfolio files are frozen after portfolio submission' if portfolio_locked? - # get path to portfolio dir portfolio_tmp_dir = portfolio_temp_path return unless Dir.exist? portfolio_tmp_dir diff --git a/app/models/project.rb b/app/models/project.rb index 0cd66417e8..288e2cd0b3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -268,25 +268,13 @@ def active? unit.active end + # True once this project's portfolio has been submitted (compiling or available) in a unit + # that locks projects on portfolio submission. Tasks and comments are frozen while true, so + # the project reflects exactly what the student had when they created their portfolio. def portfolio_locked? unit.lock_project_on_portfolio_submission? && (compile_portfolio? || portfolio_available) end - def effective_portfolio_deadline - unit.effective_portfolio_deadline_for(self) - end - - def effective_portfolio_deadline_timezone - return nil if unit.portfolio_deadline.blank? - - unit.portfolio_deadline_timezone_for(self).name - end - - def portfolio_deadline_passed?(at: Time.current) - deadline = effective_portfolio_deadline - deadline.present? && at > deadline - end - # # Get a string representation of the Target Grade # diff --git a/app/models/task.rb b/app/models/task.rb index b0391348f2..c735041e44 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -6,7 +6,7 @@ class Task < ApplicationRecord include ApplicationHelper include GradeHelper - attr_accessor :discussion_confirmed_for_transition, :portfolio_lock_bypass + attr_accessor :discussion_confirmed_for_transition # # Permissions around task data @@ -195,15 +195,10 @@ def prevent_time_exceeed_if_assess_in_portfolio_enabled def prevent_changes_when_portfolio_locked return unless project&.portfolio_locked? - return if portfolio_lock_bypass errors.add(:base, 'Task cannot be changed while the project portfolio is submitted') end - def portfolio_lock_prevents_transition?(group_transition) - project.portfolio_locked? && !group_transition && !portfolio_lock_bypass - end - def for_definition_with_quality? task_definition.has_stars? end @@ -645,7 +640,7 @@ def ensured_group_submission def trigger_transition(trigger: '', by_user: nil, bulk: false, group_transition: false, quality: 1, recursive_fix: false, check_feedback: false, system_transition: false) - if portfolio_lock_prevents_transition?(group_transition) + if project.portfolio_locked? errors.add(:base, 'Project is locked because its portfolio has been submitted') return nil end @@ -1690,8 +1685,6 @@ def stage_word_document_previews(converted_documents) # The student has uploaded new work... # def create_submission_and_trigger_state_change(user, propagate = true, contributions = nil, trigger = 'ready_for_feedback', initial_task = nil) - self.portfolio_lock_bypass = true if group_task? && initial_task.present? - if group_task? && propagate if contributions.nil? # even distribution contribs = group.projects.map { |proj| { project: proj, pct: 100 / group.projects.count, pts: 3 } } @@ -1699,7 +1692,13 @@ def create_submission_and_trigger_state_change(user, propagate = true, contribut contribs = contributions.map { |data| { project: Project.find(data[:project_id]), pct: data[:pct].to_i, pts: data[:pts].to_i } } end group_submission = group.create_submission self, "#{user.name} has submitted work", contribs - group_submission.tasks.each { |t| t.create_submission_and_trigger_state_change(user, false, contributions, trigger, self) } + group_submission.tasks.each do |t| + # Group members whose own portfolio is locked keep the exact task state + # they had when they submitted - do not propagate this submission to them. + next if t.project.portfolio_locked? + + t.create_submission_and_trigger_state_change(user, false, contributions, trigger, self) + end reload else self.file_uploaded_at = Time.zone.now @@ -1719,8 +1718,6 @@ def create_submission_and_trigger_state_change(user, propagate = true, contribut save end - ensure - self.portfolio_lock_bypass = false end # diff --git a/app/models/unit.rb b/app/models/unit.rb index 036a034acf..5dfcdb2934 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -206,8 +206,6 @@ def role_for(user) belongs_to :overseer_image, optional: true - belongs_to :portfolio_deadline_campus, class_name: 'Campus', optional: true - validates :name, :description, :start_date, :end_date, presence: true validates :name, allowed_characters: { type: :unit_name } validates :code, allowed_characters: { type: :unit_code } @@ -245,8 +243,6 @@ def role_for(user) validate :cant_disable_aip_only_if_aip_tasks_exist validate :grade_definitions_are_valid validate :configured_grades_preserve_used_values, if: :will_save_change_to_grade_values? - validate :shared_portfolio_deadline_has_campus - validate :portfolio_deadline_format_is_valid scope :current, -> { current_for_date(Time.zone.now) } scope :current_for_date, ->(date) { where('start_date <= ? AND end_date >= ?', date, date) } @@ -277,65 +273,6 @@ def discuss_timeout_warning_before_expiry errors.add(:discuss_timeout_warning_days, 'must be less than the expiry days') end - def shared_portfolio_deadline_has_campus - return if portfolio_deadline.blank? || portfolio_deadline_per_campus? - return if portfolio_deadline_campus.present? - - errors.add(:portfolio_deadline_campus, 'must be selected when one campus timezone is used for all students') - end - - def portfolio_deadline_format_is_valid - return unless @portfolio_deadline_format_invalid - - errors.add(:portfolio_deadline, 'must use the format YYYY-MM-DDTHH:mm') - end - - # Keep the API terminology while storing the value in the existing column. - def portfolio_deadline - portfolio_due_date - end - - def portfolio_deadline=(value) - @portfolio_deadline_format_invalid = false - if value.is_a?(String) && value.present? - begin - parsed = DateTime.strptime(value, '%Y-%m-%dT%H:%M') - value = Time.zone.local(parsed.year, parsed.month, parsed.day, parsed.hour, parsed.min) - rescue ArgumentError - @portfolio_deadline_format_invalid = true - value = nil - end - end - - self.portfolio_due_date = value - end - - def portfolio_deadline_timezone_for(project) - timezone_name = if portfolio_deadline_per_campus? - project&.campus&.timezone - else - portfolio_deadline_campus&.timezone - end - - ActiveSupport::TimeZone[timezone_name.presence || Time.zone.name] || Time.zone - end - - def effective_portfolio_deadline_for(project) - return nil if portfolio_deadline.blank? - - timezone = portfolio_deadline_timezone_for(project) - local_deadline = timezone.local( - portfolio_deadline.year, - portfolio_deadline.month, - portfolio_deadline.day, - portfolio_deadline.hour, - portfolio_deadline.min, - portfolio_deadline.sec - ) - - local_deadline.advance(days: project&.spec_con_days.to_i) - end - def self.notify_discuss_timeouts! set_active.find_each(&:notify_discuss_timeouts!) end @@ -700,18 +637,6 @@ def rollover(teaching_period, start_date, end_date, new_code) new_unit.portfolio_auto_generation_date = new_unit.date_for_week_and_day(week_number(self.portfolio_auto_generation_date), Date::ABBR_DAYNAMES[self.portfolio_auto_generation_date.wday]) end - if portfolio_deadline.present? - shifted_deadline = new_unit.date_for_week_and_day( - week_number(portfolio_deadline), - Date::ABBR_DAYNAMES[portfolio_deadline.wday] - ) - new_unit.portfolio_deadline = shifted_deadline.change( - hour: portfolio_deadline.hour, - min: portfolio_deadline.min, - sec: portfolio_deadline.sec - ) - end - # Clear main convenor - do not use old role id new_unit.main_convenor_id = nil diff --git a/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb b/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb index 0df133ba56..3e5a0daed3 100644 --- a/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb +++ b/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb @@ -1,7 +1,5 @@ class AddPortfolioDeadlineAndSubmissionLock < ActiveRecord::Migration[8.0] def change add_column :units, :lock_project_on_portfolio_submission, :boolean, default: false, null: false - add_column :units, :portfolio_deadline_per_campus, :boolean, default: true, null: false - add_reference :units, :portfolio_deadline_campus, null: true end end diff --git a/db/schema.rb b/db/schema.rb index a47cf5b009..3f755ffcda 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -953,12 +953,9 @@ t.integer "discuss_timeout_warning_days", default: 7, null: false t.integer "discuss_timeout_expire_days", default: 14, null: false t.boolean "lock_project_on_portfolio_submission", default: false, null: false - t.boolean "portfolio_deadline_per_campus", default: true, null: false - t.bigint "portfolio_deadline_campus_id" t.index ["draft_task_definition_id"], name: "index_units_on_draft_task_definition_id" t.index ["main_convenor_id"], name: "index_units_on_main_convenor_id" t.index ["overseer_image_id"], name: "index_units_on_overseer_image_id" - t.index ["portfolio_deadline_campus_id"], name: "index_units_on_portfolio_deadline_campus_id" t.index ["teaching_period_id"], name: "index_units_on_teaching_period_id" t.check_constraint "json_valid(`grade_values`)", name: "grade_values" end diff --git a/test/api/portfolio_submission_lock_api_test.rb b/test/api/portfolio_submission_lock_api_test.rb index c7a0344e89..ed05b408e9 100644 --- a/test/api/portfolio_submission_lock_api_test.rb +++ b/test/api/portfolio_submission_lock_api_test.rb @@ -4,7 +4,6 @@ class PortfolioSubmissionLockApiTest < ActiveSupport::TestCase include Rack::Test::Methods - include ActiveSupport::Testing::TimeHelpers include TestHelpers::AuthHelper include TestHelpers::JsonHelper @@ -12,8 +11,7 @@ def app Rails.application end - def create_portfolio_project(deadline: '2026-08-17T10:00') - campus = FactoryBot.create(:campus, timezone: 'UTC') + def create_portfolio_project(locked: true) unit = FactoryBot.create( :unit, with_students: false, @@ -22,116 +20,65 @@ def create_portfolio_project(deadline: '2026-08-17T10:00') outcome_count: 0, staff_count: 0, campus_count: 0, - lock_project_on_portfolio_submission: true, - portfolio_deadline: deadline, - portfolio_deadline_per_campus: true + lock_project_on_portfolio_submission: locked ) - project = FactoryBot.create(:project, unit: unit, campus: campus) + project = FactoryBot.create(:project, unit: unit) FileUtils.touch(project.portfolio_path) project.update!(portfolio_production_date: Time.zone.now) project end - def delete_portfolio(project, confirm_late: nil) - path = "/api/submission/project/#{project.id}/portfolio" - if confirm_late.nil? - delete path - else - delete path, { confirm_late: confirm_late }.to_json, 'CONTENT_TYPE' => 'application/json' - end + def delete_portfolio(project) + delete "/api/submission/project/#{project.id}/portfolio" end - def test_student_must_confirm_deletion_after_effective_deadline + def test_deleting_the_whole_portfolio_unlocks_the_project project = create_portfolio_project + add_auth_header_for(user: project.student) - travel_to(Time.zone.parse('2026-08-17 10:00:01 UTC')) do - add_auth_header_for(user: project.student) - delete_portfolio(project) - - assert_equal 409, last_response.status - assert last_response_body['portfolio_late_confirmation_required'] - assert_equal '2026-08-17T10:00:00Z', last_response_body['effective_portfolio_deadline'] - assert_equal 'UTC', last_response_body['effective_portfolio_deadline_timezone'] - assert project.reload.portfolio_locked? + assert project.portfolio_locked? - delete_portfolio(project, confirm_late: true) - assert_includes [200, 204], last_response.status - end + delete_portfolio(project) + assert_includes [200, 204], last_response.status assert_not project.reload.portfolio_locked? assert_not File.exist?(project.portfolio_path) assert File.exist?("#{project.portfolio_path}.old") end - def test_exact_deadline_does_not_require_late_confirmation + def test_removing_a_single_portfolio_file_is_blocked_while_locked project = create_portfolio_project - - travel_to(Time.zone.parse('2026-08-17 10:00:00 UTC')) do - add_auth_header_for(user: project.student) - delete_portfolio(project) - assert_includes [200, 204], last_response.status - end - end - - def test_compiling_portfolio_cannot_be_deleted - project = create_portfolio_project(deadline: nil) - FileUtils.rm_f(project.portfolio_path) - project.update!(compile_portfolio: true, portfolio_production_date: nil) add_auth_header_for(user: project.student) - delete_portfolio(project) - - assert_equal 409, last_response.status - assert_match(/still compiling/, last_response_body['error']) - assert project.reload.portfolio_locked? - end - - def test_tutor_cannot_delete_a_portfolio - project = create_portfolio_project(deadline: nil) - tutor = FactoryBot.create(:user, :tutor) - project.unit.employ_staff(tutor, Role.tutor) - add_auth_header_for(user: tutor) - - delete_portfolio(project) + delete "/api/submission/project/#{project.id}/portfolio", idx: 1, kind: 'document', name: 'Notes' assert_equal 403, last_response.status assert project.reload.portfolio_locked? end - def test_convenor_can_delete_a_portfolio - project = create_portfolio_project(deadline: nil) - convenor = FactoryBot.create(:user, :convenor) - project.unit.employ_staff(convenor, Role.convenor) - add_auth_header_for(user: convenor) + def test_removing_a_single_portfolio_file_is_allowed_when_not_locked + project = create_portfolio_project(locked: false) + add_auth_header_for(user: project.student) - delete_portfolio(project) + delete "/api/submission/project/#{project.id}/portfolio", idx: 1, kind: 'document', name: 'Notes' - assert_includes [200, 204], last_response.status - assert_not project.reload.portfolio_locked? + assert_not_equal 403, last_response.status end - def test_convenor_can_configure_and_read_portfolio_submission_settings - project = create_portfolio_project(deadline: nil) + def test_convenor_can_configure_the_portfolio_lock_setting + project = create_portfolio_project(locked: false) unit = project.unit - timezone_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth') add_auth_header_for(user: unit.main_convenor_user) put_json( "/api/units/#{unit.id}", - unit: { - portfolio_deadline: '2026-09-12T17:45', - portfolio_deadline_per_campus: false, - portfolio_deadline_campus_id: timezone_campus.id, - lock_project_on_portfolio_submission: true - } + unit: { lock_project_on_portfolio_submission: true } ) assert_equal 200, last_response.status, last_response_body + get "/api/units/#{unit.id}" assert_equal 200, last_response.status, last_response_body - assert_equal '2026-09-12T17:45', last_response_body['portfolio_deadline'] - assert_not last_response_body['portfolio_deadline_per_campus'] - assert_equal timezone_campus.id, last_response_body['portfolio_deadline_campus_id'] assert last_response_body['lock_project_on_portfolio_submission'] end end diff --git a/test/api/projects_api_test.rb b/test/api/projects_api_test.rb index 661b18d834..bb4a748b80 100644 --- a/test/api/projects_api_test.rb +++ b/test/api/projects_api_test.rb @@ -52,7 +52,7 @@ def test_projects_returns_correct_data # Add username and auth_token to Header add_auth_header_for(user: user) - keys = %w[id unit campus_id user_id target_grade portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed spec_con_days escalation_attempts_remaining] + keys = %w[id unit campus_id user_id target_grade portfolio_available portfolio_locked spec_con_days escalation_attempts_remaining] key_test = %w[campus_id target_grade spec_con_days] get '/api/projects' @@ -77,8 +77,8 @@ def test_get_project_response_is_correct # Add username and auth_token to Header add_auth_header_for(user: user) - keys = %w[id unit unit_id user_id campus_id target_grade submitted_grade portfolio_files compile_portfolio portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed uses_draft_learning_summary tasks tutorial_enrolments groups spec_con_days escalation_attempts_remaining] - key_test = keys - %w[unit user_id portfolio_available portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed tasks tutorial_enrolments groups] + keys = %w[id unit unit_id user_id campus_id target_grade submitted_grade portfolio_files compile_portfolio portfolio_available portfolio_locked uses_draft_learning_summary tasks tutorial_enrolments groups spec_con_days escalation_attempts_remaining] + key_test = keys - %w[unit user_id portfolio_available portfolio_locked tasks tutorial_enrolments groups] get "/api/projects/#{project.id}" assert_equal 200, last_response.status, last_response_body @@ -227,15 +227,10 @@ def test_submitted_grade_cant_change_after_submission assert_equal 200, last_response.status, last_response_body assert_equal user.projects.find(project.id).submitted_grade, 2 - keys = %w[ - campus_id target_grade submitted_grade compile_portfolio portfolio_available portfolio_locked - effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed - uses_draft_learning_summary - ] + keys = %w[campus_id target_grade submitted_grade compile_portfolio portfolio_available portfolio_locked uses_draft_learning_summary] assert_json_limit_keys_to_exactly keys, last_response_body - assert_json_matches_model project, last_response_body, - keys - %w[portfolio_locked effective_portfolio_deadline effective_portfolio_deadline_timezone portfolio_deadline_passed] + assert_json_matches_model project, last_response_body, keys - %w[portfolio_locked] DatabasePopulator.generate_portfolio(project) diff --git a/test/api/units_api_test.rb b/test/api/units_api_test.rb index 94903532e9..6031cbe9d4 100644 --- a/test/api/units_api_test.rb +++ b/test/api/units_api_test.rb @@ -329,8 +329,7 @@ def test_unit_output code id name main_convenor_id description active auto_apply_extension_before_deadline send_notifications enable_sync_enrolments enable_sync_timetable draft_task_definition_id allow_student_extension_requests extension_weeks_on_resubmit_request - allow_student_change_tutorial portfolio_deadline portfolio_deadline_per_campus - portfolio_deadline_campus_id lock_project_on_portfolio_submission + allow_student_change_tutorial lock_project_on_portfolio_submission ] assert actual_unit.key?("my_role"), actual_unit.inspect diff --git a/test/models/portfolio_submission_lock_test.rb b/test/models/portfolio_submission_lock_test.rb index bb11be314f..b9f1052242 100644 --- a/test/models/portfolio_submission_lock_test.rb +++ b/test/models/portfolio_submission_lock_test.rb @@ -11,80 +11,12 @@ def build_unit(**attributes) ) end - def test_portfolio_settings_default_to_existing_unlocked_behaviour + def test_portfolio_lock_defaults_to_off unit = build_unit - assert_nil unit.portfolio_deadline - assert unit.portfolio_deadline_per_campus? assert_not unit.lock_project_on_portfolio_submission? end - def test_shared_deadline_requires_a_campus - unit = build_unit - unit.portfolio_deadline = '2026-10-04T23:30' - unit.portfolio_deadline_per_campus = false - - assert_not unit.valid? - assert_includes unit.errors[:portfolio_deadline_campus], - 'must be selected when one campus timezone is used for all students' - - unit.portfolio_deadline_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth') - assert unit.valid?, unit.errors.full_messages.to_sentence - end - - def test_deadline_rejects_an_invalid_local_datetime - unit = build_unit - - unit.portfolio_deadline = '17 August 2026 at five' - - assert_not unit.valid? - assert_includes unit.errors[:portfolio_deadline], 'must use the format YYYY-MM-DDTHH:mm' - end - - def test_effective_deadline_uses_project_campus_and_special_consideration_calendar_days - campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') - unit = build_unit( - portfolio_deadline: '2026-10-03T23:30', - portfolio_deadline_per_campus: true - ) - project = FactoryBot.create(:project, unit: unit, campus: campus, spec_con_days: 2) - - deadline = project.effective_portfolio_deadline - - assert_equal 'Australia/Melbourne', project.effective_portfolio_deadline_timezone - assert_equal '2026-10-05T23:30:00+11:00', deadline.iso8601 - assert_not project.portfolio_deadline_passed?(at: deadline) - assert project.portfolio_deadline_passed?(at: deadline + 1.second) - end - - def test_shared_deadline_uses_selected_campus_even_when_it_is_inactive - project_campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') - deadline_campus = FactoryBot.create(:campus, timezone: 'Australia/Perth', active: false) - unit = build_unit( - portfolio_deadline: '2026-08-17T16:00', - portfolio_deadline_per_campus: false, - portfolio_deadline_campus: deadline_campus - ) - project = FactoryBot.create(:project, unit: unit, campus: project_campus) - - assert_equal 'Australia/Perth', project.effective_portfolio_deadline_timezone - assert_equal '2026-08-17T16:00:00+08:00', project.effective_portfolio_deadline.iso8601 - end - - def test_deadline_without_a_project_campus_uses_the_deployment_timezone - unit = build_unit( - portfolio_deadline: '2026-08-17T16:00', - portfolio_deadline_per_campus: true - ) - project = FactoryBot.create(:project, unit: unit, campus: nil) - - deadline = project.effective_portfolio_deadline - - assert_equal Time.zone.name, project.effective_portfolio_deadline_timezone - assert_equal [2026, 8, 17, 16, 0], - [deadline.year, deadline.month, deadline.day, deadline.hour, deadline.min] - end - def test_lock_is_immediate_retroactive_and_clears_after_compile_failure unit = build_unit(lock_project_on_portfolio_submission: false) project = FactoryBot.create(:project, unit: unit, compile_portfolio: true) @@ -129,19 +61,7 @@ def test_locked_project_rejects_task_and_comment_changes_and_destruction assert Task.exists?(task.id) end - def test_group_operations_can_use_the_explicit_bypass - unit = build_unit(lock_project_on_portfolio_submission: true) - definition = FactoryBot.create(:task_definition, unit: unit) - project = FactoryBot.create(:project, unit: unit, compile_portfolio: true) - task = FactoryBot.build(:task, project: project, task_definition: definition) - - assert_not task.save - - task.portfolio_lock_bypass = true - assert task.save, task.errors.full_messages.to_sentence - end - - def test_group_submission_updates_a_frozen_member_transactionally + def test_group_submission_skips_a_member_whose_portfolio_is_locked unit = FactoryBot.create( :unit, lock_project_on_portfolio_submission: true, @@ -158,31 +78,14 @@ def test_group_submission_updates_a_frozen_member_transactionally submitter_project, frozen_project = group.projects.to_a submitter_task = submitter_project.task_for_task_definition(unit.task_definitions.first) frozen_task = frozen_project.task_for_task_definition(unit.task_definitions.first) + frozen_task_status_id = frozen_task.task_status_id frozen_project.update!(compile_portfolio: true) contributions = group.projects.map { |project| { project: project, pct: 50, pts: 3 } } - submission = group.create_submission(submitter_task, 'Group submission', contributions) + group.create_submission(submitter_task, 'Group submission', contributions) - assert_equal submission, frozen_task.reload.group_submission - assert_equal 50, frozen_task.contribution_pct + assert_nil frozen_task.reload.group_submission + assert_equal frozen_task_status_id, frozen_task.task_status_id assert frozen_project.reload.portfolio_locked? end - - def test_lock_authorisation_applies_to_students_tutors_and_convenors_but_not_portfolio_assessment - unit = build_unit(lock_project_on_portfolio_submission: true) - definition = FactoryBot.create(:task_definition, unit: unit) - project = FactoryBot.create(:project, unit: unit) - task = FactoryBot.create(:task, project: project, task_definition: definition) - tutor = FactoryBot.create(:user, :tutor) - convenor = FactoryBot.create(:user, :convenor) - unit.employ_staff(tutor, Role.tutor) - unit.employ_staff(convenor, Role.convenor) - project.update!(compile_portfolio: true) - - assert_not AuthorisationHelpers.authorise?(project.student, task, :make_submission) - assert_not AuthorisationHelpers.authorise?(tutor, task, :make_submission) - assert_not AuthorisationHelpers.authorise?(convenor, task, :make_submission) - assert_not AuthorisationHelpers.authorise?(project.student, project, :change) - assert AuthorisationHelpers.authorise?(convenor, project, :assess) - end end diff --git a/test/models/unit_model_test.rb b/test/models/unit_model_test.rb index 2ceb052faa..151415ff84 100644 --- a/test/models/unit_model_test.rb +++ b/test/models/unit_model_test.rb @@ -217,29 +217,6 @@ def test_rollover_of_portfolio_generation unit2.destroy end - def test_rollover_retains_and_shifts_portfolio_submission_settings - @unit.update!(portfolio_auto_generation_date: nil) - original_deadline = (@unit.start_date + 3.weeks + 2.days).change(hour: 14, min: 35) - deadline_campus = Campus.first - @unit.update!( - portfolio_deadline: original_deadline.strftime('%Y-%m-%dT%H:%M'), - portfolio_deadline_per_campus: false, - portfolio_deadline_campus: deadline_campus, - lock_project_on_portfolio_submission: true - ) - - unit2 = @unit.rollover TeachingPeriod.find(2), nil, nil, nil - - assert unit2.lock_project_on_portfolio_submission? - refute unit2.portfolio_deadline_per_campus? - assert_equal deadline_campus, unit2.portfolio_deadline_campus - assert_equal @unit.week_number(original_deadline), unit2.week_number(unit2.portfolio_deadline) - assert_equal original_deadline.wday, unit2.portfolio_deadline.wday - assert_equal [14, 35], [unit2.portfolio_deadline.hour, unit2.portfolio_deadline.min] - - unit2.destroy - end - def test_rollover_of_group_tasks unit = FactoryBot.create(:unit, code: 'SIT102', diff --git a/test/sidekiq/execute_communication_set_job_test.rb b/test/sidekiq/execute_communication_set_job_test.rb index e93bea9a1a..8bad3d7e1d 100644 --- a/test/sidekiq/execute_communication_set_job_test.rb +++ b/test/sidekiq/execute_communication_set_job_test.rb @@ -83,7 +83,7 @@ def test_task_comment_action_adds_a_comment_to_each_selected_students_task assert_equal comment_author, comment_one.user assert_equal comment_author, comment_two.user - assert_equal "Please review Ada for #{unit.code}", comment_one.comment - assert_equal "Please review Grace for #{unit.code}", comment_two.comment + assert_equal 'Please review Ada for ' + unit.code, comment_one.comment + assert_equal 'Please review Grace for ' + unit.code, comment_two.comment end end