Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/api/entities/minimal/minimal_unit_entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ class MinimalUnitEntity < Grape::Entity
with_options(format_with: :date_only) do
expose :start_date
expose :end_date
expose :last_teaching_date
end

expose :active
expose :grade_values
expose :grade_definitions
expose :discuss_timeout_enabled, unless: :summary_only
expose :discuss_timeout_active?, as: :discuss_timeout_active, unless: :summary_only
expose :discuss_timeout_expire_days, unless: :summary_only
end
end
Expand Down
1 change: 1 addition & 0 deletions app/api/entities/teaching_period_entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class TeachingPeriodEntity < Grape::Entity
expose :year
expose :start_date
expose :end_date
expose :last_teaching_date
expose :active_until
expose :active do |teaching_period, options|
teaching_period.active?
Expand Down
2 changes: 2 additions & 0 deletions app/api/entities/unit_entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def can_read_unit_config?(my_role)
with_options(format_with: :date_only) do
expose :start_date
expose :end_date
expose :last_teaching_date
expose :portfolio_auto_generation_date, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }, expose_nil: false
end

Expand Down Expand Up @@ -88,6 +89,7 @@ def can_read_unit_config?(my_role)
expose :feedback_overflow_threshold_days, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }

expose :discuss_timeout_enabled, unless: :summary_only
expose :discuss_timeout_active?, as: :discuss_timeout_active, unless: :summary_only
expose :discuss_timeout_warning_days, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }
expose :discuss_timeout_expire_days, unless: :summary_only

Expand Down
4 changes: 4 additions & 0 deletions app/api/teaching_periods_authenticated_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class TeachingPeriodsAuthenticatedApi < Grape::API
requires :year, type: Integer, desc: 'The year of the teaching period'
requires :start_date, type: Date, desc: 'The start date of the teaching period'
requires :end_date, type: Date, desc: 'The end date of the teaching period'
optional :last_teaching_date, type: Date, desc: 'The final day of teaching, inclusive'
requires :active_until, type: Date, desc: 'The teaching period will be active until this date'
end
end
Expand All @@ -28,6 +29,7 @@ class TeachingPeriodsAuthenticatedApi < Grape::API
:year,
:start_date,
:end_date,
:last_teaching_date,
:active_until)

result = TeachingPeriod.create!(teaching_period_parameters)
Expand All @@ -47,6 +49,7 @@ class TeachingPeriodsAuthenticatedApi < Grape::API
optional :year, type: Integer, desc: 'The year of the teaching period'
optional :start_date, type: Date, desc: 'The start date of the teaching period'
optional :end_date, type: Date, desc: 'The end date of the teaching period'
optional :last_teaching_date, type: Date, desc: 'The final day of teaching, inclusive'
optional :active_until, type: Date, desc: 'The teaching period will be active until this date'
end
end
Expand All @@ -61,6 +64,7 @@ class TeachingPeriodsAuthenticatedApi < Grape::API
:year,
:start_date,
:end_date,
:last_teaching_date,
:active_until)

teaching_period.update!(teaching_period_parameters)
Expand Down
12 changes: 10 additions & 2 deletions app/api/units_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class UnitsApi < Grape::API
authenticated?

if params[:unit]
for key in [:start_date, :end_date] do
for key in [:start_date, :end_date, :last_teaching_date] do
if params[:unit][key].present?
date_val = DateTime.parse(params[:unit][key])
params[:unit][key] = date_val
Expand Down Expand Up @@ -77,6 +77,7 @@ class UnitsApi < Grape::API
optional :teaching_period_id, type: Integer
optional :start_date, type: Date
optional :end_date, type: Date
optional :last_teaching_date, type: Date
optional :main_convenor_id, type: Integer
optional :auto_apply_extension_before_deadline, type: Boolean, desc: 'Indicates if extensions before the deadline should be automatically applied'
optional :mark_late_submissions_as_assess_in_portfolio, type: Boolean, desc: 'Indicates if late submissions should be set Time Exceeded or Assess in Portfolio'
Expand Down Expand Up @@ -106,6 +107,7 @@ class UnitsApi < Grape::API

mutually_exclusive :teaching_period_id, :start_date
mutually_exclusive :teaching_period_id, :end_date
mutually_exclusive :teaching_period_id, :last_teaching_date
end
end
put '/units/:id' do
Expand All @@ -120,6 +122,7 @@ class UnitsApi < Grape::API
:description,
:start_date,
:end_date,
:last_teaching_date,
:teaching_period_id,
:active,
:main_convenor_id,
Expand Down Expand Up @@ -177,6 +180,7 @@ class UnitsApi < Grape::API
optional :teaching_period_id, type: Integer
optional :start_date, type: Date
optional :end_date, type: Date
optional :last_teaching_date, type: Date
optional :main_convenor_user_id, type: Integer
optional :auto_apply_extension_before_deadline, type: Boolean, desc: 'Indicates if extensions before the deadline should be automatically applied', default: true
optional :mark_late_submissions_as_assess_in_portfolio, type: Boolean, desc: 'Indicates if late submissions should be set to Time Exceeded or Assess in Portfolio', default: true
Expand All @@ -203,6 +207,7 @@ class UnitsApi < Grape::API

mutually_exclusive :teaching_period_id, :start_date
mutually_exclusive :teaching_period_id, :end_date
mutually_exclusive :teaching_period_id, :last_teaching_date
all_or_none_of :start_date, :end_date
end
end
Expand All @@ -220,6 +225,7 @@ class UnitsApi < Grape::API
:description,
:start_date,
:end_date,
:last_teaching_date,
:auto_apply_extension_before_deadline,
:mark_late_submissions_as_assess_in_portfolio,
:send_notifications,
Expand Down Expand Up @@ -288,10 +294,12 @@ class UnitsApi < Grape::API
optional :teaching_period_id, type: Integer, desc: 'The teaching period to rollover to'
optional :start_date, type: Date, desc: 'The start date of the new unit'
optional :end_date, type: Date, desc: 'The end date of the new unit'
optional :last_teaching_date, type: Date, desc: 'The final day of teaching for a unit without a teaching period'
optional :new_unit_code, type: String, desc: 'The unit code for the new unit'

exactly_one_of :teaching_period_id, :start_date
all_or_none_of :start_date, :end_date
mutually_exclusive :teaching_period_id, :last_teaching_date
end
post '/units/:id/rollover' do
unit = Unit.find(params[:id])
Expand All @@ -306,7 +314,7 @@ class UnitsApi < Grape::API
tp = TeachingPeriod.find(teaching_period_id)
result = unit.rollover(tp, nil, nil, params[:new_unit_code])
else
result = unit.rollover(nil, params[:start_date], params[:end_date], params[:new_unit_code])
result = unit.rollover(nil, params[:start_date], params[:end_date], params[:new_unit_code], params[:last_teaching_date])
end

my_role = result.role_for(current_user)
Expand Down
28 changes: 25 additions & 3 deletions app/models/teaching_period.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@ class TeachingPeriod < ApplicationRecord
inclusion: { in: 2000..2999, message: "%{value} is not a valid year" }
validates :start_date, presence: true
validates :end_date, presence: true
validates :last_teaching_date, presence: true
validates :active_until, presence: true

validate :validate_end_date_after_start_date, :validate_active_until_after_end_date
before_validation :default_last_teaching_date, on: :create
before_validation :sync_last_teaching_date_with_end_date, on: :update

validate :validate_end_date_after_start_date, :validate_last_teaching_date, :validate_active_until_after_end_date

after_update :propogate_date_changes
after_update :refresh_communication_schedule_caches, if: :saved_change_to_teaching_dates?
Expand Down Expand Up @@ -190,11 +194,29 @@ def validate_end_date_after_start_date
end
end

def default_last_teaching_date
self.last_teaching_date ||= end_date&.to_date
end

def sync_last_teaching_date_with_end_date
return unless will_save_change_to_end_date? && !will_save_change_to_last_teaching_date?
return unless last_teaching_date == end_date_change_to_be_saved.first&.to_date

self.last_teaching_date = end_date&.to_date
end

def validate_last_teaching_date
return if start_date.blank? || end_date.blank? || last_teaching_date.blank?
return if last_teaching_date.between?(start_date.to_date, end_date.to_date)

errors.add(:last_teaching_date, 'must be between the start and end dates')
end

def propogate_date_changes
return unless saved_change_to_start_date? || saved_change_to_end_date?
return unless saved_change_to_start_date? || saved_change_to_end_date? || saved_change_to_last_teaching_date?

units.each do |u|
u.update(start_date: self.start_date, end_date: self.end_date)
u.update!(start_date: start_date, end_date: end_date, last_teaching_date: last_teaching_date)
end
end

Expand Down
53 changes: 47 additions & 6 deletions app/models/unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@

belongs_to :overseer_image, optional: true

validates :name, :description, :start_date, :end_date, presence: true
validates :name, :description, :start_date, :end_date, :last_teaching_date, presence: true
validates :name, allowed_characters: { type: :unit_name }
validates :code, allowed_characters: { type: :unit_code }

Expand All @@ -237,7 +237,11 @@
validate :warning_not_greater_than_overflow
validate :discuss_timeout_warning_before_expiry

before_validation :default_last_teaching_date, on: :create
before_validation :sync_last_teaching_date_with_end_date, on: :update

validate :validate_end_date_after_start_date
validate :validate_last_teaching_date
validate :ensure_teaching_period_dates_match, if: :has_teaching_period?

validate :ensure_main_convenor_is_appropriate, if: :main_convenor_id_changed?
Expand All @@ -259,6 +263,9 @@
scope :within_teaching_dates, lambda { |now = Time.zone.now|
set_active.where('units.end_date IS NULL OR units.end_date >= ?', now.to_date)
}
scope :within_last_teaching_date, lambda { |now = Time.zone.now|
set_active.where('units.last_teaching_date >= ?', now.to_date)
}

include UnitTiiModule

Expand Down Expand Up @@ -287,13 +294,20 @@
active? && (end_date.blank? || now.to_date <= end_date)
end

def within_last_teaching_date?(now = Time.zone.now)
active? && now.to_date <= last_teaching_date
end

def discuss_timeout_active?
discuss_timeout_enabled && within_last_teaching_date?
end

def self.notify_discuss_timeouts!
within_teaching_dates.find_each(&:notify_discuss_timeouts!)
within_last_teaching_date.find_each(&:notify_discuss_timeouts!)
end

def notify_discuss_timeouts!
return 0 unless discuss_timeout_enabled
return 0 unless within_teaching_dates?
return 0 unless discuss_timeout_active?

discuss_timeout_tasks.find_each.sum do |task|
notify_discuss_timeout_for(task)
Expand All @@ -310,12 +324,15 @@

def notify_discuss_timeout_for(task, teaching_breaks: nil, now_time: Time.zone.now)
return 0 if task.moved_to_discuss_at.blank?
return 0 if now_time.to_date > last_teaching_date

expiry_at = task.discuss_timeout_expiry_at(discuss_timeout_expire_days, teaching_breaks: teaching_breaks)
return 0 if expiry_at.to_date >= last_teaching_date

actor = task.project.tutor_for(task.task_definition) || main_convenor&.user
return 0 if actor.blank?

elapsed_days = task.discuss_timeout_elapsed_days(now_time, teaching_breaks: teaching_breaks)
expiry_at = task.discuss_timeout_expiry_at(discuss_timeout_expire_days, teaching_breaks: teaching_breaks)
if now_time.in_time_zone(expiry_at.time_zone).to_date > expiry_at.to_date
expire_discuss_timeout_task(task, actor)
elsif elapsed_days >= discuss_timeout_warning_days
Expand Down Expand Up @@ -456,6 +473,7 @@
if tp.present?
self[:start_date] = tp.start_date
self[:end_date] = tp.end_date
self[:last_teaching_date] = tp.last_teaching_date
self[:teaching_period_id] = tp.id
end
super(tp)
Expand Down Expand Up @@ -519,11 +537,14 @@

def ensure_teaching_period_dates_match
if self[:start_date] != teaching_period.start_date
errors.add(:start_date, "should match teaching period date")

Check failure on line 540 in app/models/unit.rb

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Define a constant instead of duplicating this literal "should match teaching period date" 3 times.

See more on https://sonarcloud.io/project/issues?id=doubtfire-lms_doubtfire-api&issues=AaFgzMb0q_w0eho_L7JS&open=AaFgzMb0q_w0eho_L7JS&pullRequest=716
end
if self[:end_date] != teaching_period.end_date
errors.add(:end_date, "should match teaching period date")
end
if last_teaching_date != teaching_period.last_teaching_date
errors.add(:last_teaching_date, "should match teaching period date")
end
end

def ensure_main_convenor_is_appropriate
Expand Down Expand Up @@ -624,13 +645,31 @@
end
end

def default_last_teaching_date

Check warning on line 648 in app/models/unit.rb

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the callback method 'default_last_teaching_date' private.

See more on https://sonarcloud.io/project/issues?id=doubtfire-lms_doubtfire-api&issues=AaFgzMb0q_w0eho_L7JT&open=AaFgzMb0q_w0eho_L7JT&pullRequest=716
self.last_teaching_date ||= teaching_period&.last_teaching_date || end_date&.to_date
end

def sync_last_teaching_date_with_end_date

Check warning on line 652 in app/models/unit.rb

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the callback method 'sync_last_teaching_date_with_end_date' private.

See more on https://sonarcloud.io/project/issues?id=doubtfire-lms_doubtfire-api&issues=AaFg8yaHz46gQo4pBPGP&open=AaFg8yaHz46gQo4pBPGP&pullRequest=716
return unless will_save_change_to_end_date? && !will_save_change_to_last_teaching_date?
return unless last_teaching_date == end_date_change_to_be_saved.first&.to_date

self.last_teaching_date = end_date&.to_date
end

def validate_last_teaching_date
return if start_date.blank? || end_date.blank? || last_teaching_date.blank?
return if last_teaching_date.between?(start_date.to_date, end_date.to_date)

errors.add(:last_teaching_date, 'must be between the start and end dates')
end

def autogen_date_within_unit_active_period
if [start_date, end_date, portfolio_auto_generation_date].all?(&:present?) && !(start_date < portfolio_auto_generation_date && portfolio_auto_generation_date <= end_date)
errors.add(:portfolio_auto_generation_date, "should be after unit start date and before unit end date")
end
end

def rollover(teaching_period, start_date, end_date, new_code)
def rollover(teaching_period, start_date, end_date, new_code, last_teaching_date = nil)
new_unit = self.dup

new_unit.code = new_code if new_code.present?
Expand All @@ -641,6 +680,7 @@
new_unit.teaching_period = nil
new_unit.start_date = start_date
new_unit.end_date = end_date
new_unit.last_teaching_date = last_teaching_date || end_date&.to_date
end

# Clear archived
Expand Down Expand Up @@ -3853,6 +3893,7 @@

def send_weekly_status_emails(summary_stats)
return unless send_notifications
return unless within_last_teaching_date?

summary_stats[:unit] = self
summary_stats[:tutorials] = {}
Expand Down
1 change: 1 addition & 0 deletions app/sidekiq/send_discuss_timeout_email_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ def perform(task_id, sender_id, notification_type, expiry_date = nil)
task = Task.find_by(id: task_id)
sender = User.find_by(id: sender_id)
return if task.blank? || sender.blank?
return unless task.unit.discuss_timeout_active?
return unless task.unit.send_notifications
return unless task.project.student.receive_feedback_notifications

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class AddLastTeachingDateToTeachingPeriodsAndUnits < ActiveRecord::Migration[8.0]
def up
add_column :teaching_periods, :last_teaching_date, :date
add_column :units, :last_teaching_date, :date

execute 'UPDATE teaching_periods SET last_teaching_date = DATE(end_date)'
execute 'UPDATE units SET last_teaching_date = DATE(end_date)'

change_column_null :teaching_periods, :last_teaching_date, false
change_column_null :units, :last_teaching_date, false
end

def down
remove_column :units, :last_teaching_date
remove_column :teaching_periods, :last_teaching_date
end
end
4 changes: 3 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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_10_15_040755) do
ActiveRecord::Schema[8.0].define(version: 2026_10_23_034838) 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
Expand Down Expand Up @@ -802,6 +802,7 @@
t.datetime "active_until", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.date "last_teaching_date", null: false
t.index ["period", "year"], name: "index_teaching_periods_on_period_and_year", unique: true
end

Expand Down Expand Up @@ -1017,6 +1018,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.date "last_teaching_date", 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"
Expand Down
Loading
Loading