diff --git a/app/api/discussion_comment_api.rb b/app/api/discussion_comment_api.rb index ffd70117f..d619d7cc9 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 ebe150267..f760249c0 100644 --- a/app/api/entities/project_entity.rb +++ b/app/api/entities/project_entity.rb @@ -16,6 +16,12 @@ 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 + # 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 c820f2e36..f73f757ae 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -62,6 +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 + # 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 61a66c1fd..971a929a2 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -165,7 +165,21 @@ 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, + :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 aec64c65e..618857176 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] @@ -55,12 +59,20 @@ class PortfolioApi < Grape::API # 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 + portfolio_production_date: nil, + compile_portfolio: false }) project.remove_portfolio # returns details of file elsif !(params[:idx].nil? || params[:name].nil? || params[:kind].nil?) + if project.portfolio_locked? + error!({ error: 'Portfolio evidence is frozen because the portfolio has already been submitted.' }, 403) + end + idx = params[:idx] name = params[:name] kind = params[:kind] diff --git a/app/api/tasks_api.rb b/app/api/tasks_api.rb index acb4e0229..7c7364fcc 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 89e0105ee..7567be0bc 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -85,6 +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 :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' @@ -130,6 +131,7 @@ class UnitsApi < Grape::API :enable_sync_enrolments, :draft_task_definition_id, :portfolio_auto_generation_date, + :lock_project_on_portfolio_submission, :allow_flexible_dates, :allow_student_extension_requests, :extension_weeks_on_resubmit_request, @@ -187,6 +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 :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' @@ -229,6 +232,7 @@ class UnitsApi < Grape::API :allow_student_extension_requests, :extension_weeks_on_resubmit_request, :portfolio_auto_generation_date, + :lock_project_on_portfolio_submission, :allow_student_change_tutorial, :feedback_warning_threshold_days, :feedback_overflow_threshold_days, diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c74883d01..5c4558c05 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,24 @@ 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? + + 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? + + 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 fec42947a..f13451c26 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -242,7 +242,9 @@ def create_submission(submitter_task, notes, contributors) 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? if contrib[:pct].to_i > 0 task.group_submission = gs diff --git a/app/models/group_submission.rb b/app/models/group_submission.rb index 5016aab97..6e04bc7cb 100644 --- a/app/models/group_submission.rb +++ b/app/models/group_submission.rb @@ -17,10 +17,13 @@ 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 - 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}" end @@ -29,6 +32,7 @@ class GroupSubmission < ApplicationRecord 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 + next if task.project.portfolio_locked? if task != initial_task task.extensions = initial_task.extensions unless initial_task.extensions < task.extensions @@ -39,6 +43,8 @@ def propagate_transition(initial_task, trigger, by_user, quality) def propagate_grade(initial_task, new_grade, ui) tasks.each do |task| + next if task.project.portfolio_locked? + if task != initial_task task.grade_task new_grade, ui, grading_group = true end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index d01a3210e..de94c4ba8 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 eab5a5292..7448ee1fc 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! diff --git a/app/models/project.rb b/app/models/project.rb index 19dcf40db..288e2cd0b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -268,6 +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 + # # Get a string representation of the Target Grade # @@ -537,6 +544,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 3091ef372..c735041e4 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -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,12 @@ def prevent_time_exceeed_if_assess_in_portfolio_enabled end end + def prevent_changes_when_portfolio_locked + return unless project&.portfolio_locked? + + errors.add(:base, 'Task cannot be changed while the project portfolio is submitted') + end + def for_definition_with_quality? task_definition.has_stars? end @@ -632,6 +640,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 project.portfolio_locked? + 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 +1427,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 @@ -1671,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 diff --git a/app/models/task_definition.rb b/app/models/task_definition.rb index 21789ee38..a1b6c388e 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 5637e0b03..5dfcdb293 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -294,6 +294,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 @@ -3984,6 +3985,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 59ddd753f..ddbe0d7fd 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 5f99bd3fe..e748203b2 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 ad3d8abf7..5239351f9 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/20260818003147_add_portfolio_deadline_and_submission_lock.rb b/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb new file mode 100644 index 000000000..3e5a0daed --- /dev/null +++ b/db/migrate/20260818003147_add_portfolio_deadline_and_submission_lock.rb @@ -0,0 +1,5 @@ +class AddPortfolioDeadlineAndSubmissionLock < ActiveRecord::Migration[8.0] + def change + add_column :units, :lock_project_on_portfolio_submission, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 81e3ffda6..3f755ffcd 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_055309) 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 @@ -952,6 +952,7 @@ 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.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" 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 000000000..ed05b408e --- /dev/null +++ b/test/api/portfolio_submission_lock_api_test.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'test_helper' + +class PortfolioSubmissionLockApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def create_portfolio_project(locked: true) + 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: locked + ) + 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) + delete "/api/submission/project/#{project.id}/portfolio" + end + + def test_deleting_the_whole_portfolio_unlocks_the_project + project = create_portfolio_project + add_auth_header_for(user: project.student) + + assert project.portfolio_locked? + + 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_removing_a_single_portfolio_file_is_blocked_while_locked + project = create_portfolio_project + add_auth_header_for(user: project.student) + + 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_removing_a_single_portfolio_file_is_allowed_when_not_locked + project = create_portfolio_project(locked: false) + add_auth_header_for(user: project.student) + + delete "/api/submission/project/#{project.id}/portfolio", idx: 1, kind: 'document', name: 'Notes' + + assert_not_equal 403, last_response.status + end + + def test_convenor_can_configure_the_portfolio_lock_setting + project = create_portfolio_project(locked: false) + unit = project.unit + add_auth_header_for(user: unit.main_convenor_user) + + put_json( + "/api/units/#{unit.id}", + 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 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 b6a41ccc7..bb4a748b8 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 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 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,10 +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 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 + 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 8888ba3ad..6031cbe9d 100644 --- a/test/api/units_api_test.rb +++ b/test/api/units_api_test.rb @@ -325,7 +325,12 @@ 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 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 000000000..b9f105224 --- /dev/null +++ b/test/models/portfolio_submission_lock_test.rb @@ -0,0 +1,91 @@ +# 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_lock_defaults_to_off + unit = build_unit + + assert_not unit.lock_project_on_portfolio_submission? + 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_submission_skips_a_member_whose_portfolio_is_locked + 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_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 } } + + group.create_submission(submitter_task, 'Group submission', contributions) + + 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 +end diff --git a/test/sidekiq/execute_communication_set_job_test.rb b/test/sidekiq/execute_communication_set_job_test.rb index 8ec1afb8b..8bad3d7e1 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,