Skip to content
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### 🚨 Breaking changes

### ✨ New features and improvements
- Added datetime-based visibility scheduling for assessments with `visible_on` and `visible_until` columns (#7697)
- Added new loading spinner icon for tables (#7602)
- Update message and page displaying cannot create new course via external LTI tool (#7669)
- Provide file viewer the option to render Microsoft files (#7676)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/assignments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class AssignmentsController < MainApiController
:student_form_groups, :remark_due_date, :remark_message,
:assign_graders_to_criteria, :enable_test, :enable_student_tests, :allow_remarks,
:display_grader_names_to_students, :group_name_autogenerated,
:repository_folder, :is_hidden, :vcs_submit, :token_period,
:repository_folder, :is_hidden, :visible_on, :visible_until, :vcs_submit, :token_period,
:non_regenerating_tokens, :unlimited_tokens, :token_start_date, :token_end_date, :has_peer_review,
:starter_file_type, :default_starter_file_group_id].freeze

Expand Down
3 changes: 2 additions & 1 deletion app/controllers/api/grade_entry_forms_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Api
class GradeEntryFormsController < MainApiController
DEFAULT_FIELDS = [:id, :short_identifier, :description, :due_date, :is_hidden, :show_total].freeze
DEFAULT_FIELDS = [:id, :short_identifier, :description, :due_date, :is_hidden, :visible_on, :visible_until,
:show_total].freeze

# Sends the contents of the specified grade entry form
# Requires: id
Expand Down
37 changes: 33 additions & 4 deletions app/lib/repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,17 +249,46 @@ def self.get_repo_auth_records
# Return a nested hash of the form { assignment_id => { section_id => visibility } } where visibility
# is a boolean indicating whether the given assignment is visible to the given section.
def self.visibility_hash
current_time = Time.current
records = Assignment.left_outer_joins(:assessment_section_properties)
.pluck_to_hash('assessments.id',
'section_id',
'assessments.is_hidden',
'assessment_section_properties.is_hidden')
'assessments.visible_on',
'assessments.visible_until',
'assessment_section_properties.is_hidden',
'assessment_section_properties.visible_on',
'assessment_section_properties.visible_until')

visibilities = records.uniq { |r| r['assessments.id'] }
.map { |r| [r['assessments.id'], Hash.new { !r['assessments.is_hidden'] }] }
.map do |r|
# Check if datetime-based visibility is set
visible_on = r['assessments.visible_on']
visible_until = r['assessments.visible_until']
default_visible = if visible_on || visible_until
(visible_on.nil? || visible_on <= current_time) &&
(visible_until.nil? || visible_until >= current_time)
else
!r['assessments.is_hidden']
end
[r['assessments.id'], Hash.new { default_visible }]
end
.to_h

records.each do |r|
unless r['assessment_section_properties.is_hidden'].nil?
visibilities[r['assessments.id']][r['section_id']] = !r['assessment_section_properties.is_hidden']
section_visible_on = r['assessment_section_properties.visible_on']
section_visible_until = r['assessment_section_properties.visible_until']
section_is_hidden = r['assessment_section_properties.is_hidden']

unless section_is_hidden.nil? && section_visible_on.nil? && section_visible_until.nil?
# Section-specific settings exist
section_visible = if section_visible_on || section_visible_until
(section_visible_on.nil? || section_visible_on <= current_time) &&
(section_visible_until.nil? || section_visible_until >= current_time)
else
!section_is_hidden
end
visibilities[r['assessments.id']][r['section_id']] = section_visible
end
end
visibilities
Expand Down
10 changes: 10 additions & 0 deletions app/models/assessment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@ class Assessment < ApplicationRecord
# date: true maps to DateValidator (custom_name: true maps to CustomNameValidator)
# Look in lib/validators/* for more info
validates :due_date, date: true
validates :visible_on, date: true, allow_nil: true
validates :visible_until, date: true, allow_nil: true

validates :short_identifier, uniqueness: { scope: :course_id }
validates :short_identifier, presence: true
validate :short_identifier_unchanged, on: :update
validate :visible_dates_are_valid
validates :description, presence: true
validates :is_hidden, inclusion: { in: [true, false] }
validates :short_identifier, format: { with: /\A[a-zA-Z0-9\-_]+\z/ }
Expand All @@ -38,6 +41,13 @@ def short_identifier_unchanged
false
end

def visible_dates_are_valid
return if visible_on.nil? || visible_until.nil?
if visible_on >= visible_until
errors.add(:visible_until, 'must be after visible_on')
end
end

def upcoming(*)
return true if self.due_date.nil?
self.due_date > Time.current
Expand Down
11 changes: 11 additions & 0 deletions app/models/assessment_section_properties.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ class AssessmentSectionProperties < ApplicationRecord

has_one :course, through: :assessment
validate :courses_should_match
validates :visible_on, date: true, allow_nil: true
validates :visible_until, date: true, allow_nil: true
validate :visible_dates_are_valid

# Returns the dute date for a section of an assignment. Defaults to the global
# due date of the assignment.
def self.due_date_for(section, assignment)
Expand All @@ -14,4 +18,11 @@ def self.due_date_for(section, assignment)
where(section_id: section.id, assessment_id: assignment.id).first
section_due_date.try(:due_date) || assignment.due_date
end

def visible_dates_are_valid
return if visible_on.nil? || visible_until.nil?
if visible_on >= visible_until
errors.add(:visible_until, 'must be after visible_on')
end
end
end
6 changes: 5 additions & 1 deletion app/models/assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class Assignment < Assessment
:enable_student_tests, :allow_remarks,
:display_grader_names_to_students,
:display_median_to_students, :group_name_autogenerated,
:is_hidden, :vcs_submit, :has_peer_review].freeze
:is_hidden, :visible_on, :visible_until, :vcs_submit, :has_peer_review].freeze

STARTER_FILES_DIR = (
Settings.file_storage.starter_files || File.join(Settings.file_storage.default_root_path, 'starter_files')
Expand Down Expand Up @@ -1517,7 +1517,11 @@ def update_repo_required_files
# Returns whether the visibility for this assignment changed after a save.
def visibility_changed?
saved_change_to_is_hidden? ||
saved_change_to_visible_on? ||
saved_change_to_visible_until? ||
assessment_section_properties.any?(&:is_hidden_previously_changed?) ||
assessment_section_properties.any?(&:visible_on_previously_changed?) ||
assessment_section_properties.any?(&:visible_until_previously_changed?) ||
@prev_assessment_section_property_ids != self.reload.assessment_section_properties.ids
end
end
78 changes: 66 additions & 12 deletions app/models/student.rb
Original file line number Diff line number Diff line change
Expand Up @@ -256,26 +256,80 @@ def grace_credits_used_for(assessment)
grouping.grace_period_deduction_single
end

# Determine what assessments are visible to the role.
# By default, returns all assessments visible to the role for the current course.
# Optional parameter assessment_type takes values "Assignment" or "GradeEntryForm". If passed one of these options,
# only returns assessments of that type. Otherwise returns all assessment types.
# Optional parameter assessment_id: if passed an assessment id, returns a collection containing
# only the assessment with the given id, if it is visible to the current user.
# If it is not visible, returns an empty collection.
# Returns assessments visible to this student.
# Can filter by assessment_type ("Assignment" or "GradeEntryForm") and/or assessment_id.
#
# Visibility logic:
# - visible_on/visible_until datetime columns override is_hidden when set
# - Section-specific settings override global settings
# - Assessment is visible if current time is within the datetime range
def visible_assessments(assessment_type: nil, assessment_id: nil)
visible = self.assessments.where(type: assessment_type || Assessment.type)
visible = visible.where(id: assessment_id) if assessment_id
current_time = Time.current

if self.section_id
visible = visible.left_outer_joins(:assessment_section_properties)
.where('assessment_section_properties.section_id': [self.section_id, nil])
visible = visible.where('assessment_section_properties.is_hidden': false)
.or(visible.where('assessment_section_properties.is_hidden': nil,
'assessments.is_hidden': false))

# Check section-specific visibility first (takes precedence when any section property is set)
section_visible = visible.where('assessment_section_properties.section_id': self.section_id)
.where.not(assessment_section_properties: { is_hidden: nil })
.or(visible.where('assessment_section_properties.section_id': self.section_id)
.where.not(assessment_section_properties: { visible_on: nil }))
.or(visible.where('assessment_section_properties.section_id': self.section_id)
.where.not(assessment_section_properties: { visible_until: nil }))

# Make sure current time is within the datetime range
section_visible = section_visible
.where(assessment_section_properties: { visible_on: nil })
.or(section_visible.where(assessment_section_properties: { visible_on: ..current_time }))
section_visible = section_visible
.where(assessment_section_properties: { visible_until: nil })
.or(section_visible.where(assessment_section_properties: { visible_until: current_time.. }))

# Use datetime if set, otherwise fall back to is_hidden
section_visible = section_visible
.where.not(assessment_section_properties: { visible_on: nil })
.or(section_visible.where.not(assessment_section_properties: { visible_until: nil }))
.or(section_visible.where(assessment_section_properties: { is_hidden: false }))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something subtle here with the is_hidden section property.

In the case that for the section properties, visible_on: nil, visible_until: nil, is_hidden: true, then the assignment should not be visible to students in that section, regardless of what the global properties are. I don't think this is currently being implemented correctly by this logic.

You handled this differently in the visibility_hash method by first checking whether any of the three section properties were non-nil before overriding the global properties.

And in the SQL function, you're only checking assessment_section_properties.is_hidden IS NOT NULL but should probably be checking all three.


# Check global visibility (when no section-specific settings exist)
global_visible = visible.where('assessment_section_properties.section_id': nil)
.or(visible.where(assessment_section_properties: { is_hidden: nil,
visible_on: nil,
visible_until: nil }))

# Same datetime range check for global settings
global_visible = global_visible
.where(assessments: { visible_on: nil })
.or(global_visible.where(assessments: { visible_on: ..current_time }))
global_visible = global_visible
.where(assessments: { visible_until: nil })
.or(global_visible.where(assessments: { visible_until: current_time.. }))

# Use datetime if set, otherwise fall back to is_hidden
global_visible = global_visible
.where.not(assessments: { visible_on: nil })
.or(global_visible.where.not(assessments: { visible_until: nil }))
.or(global_visible.where(assessments: { is_hidden: false }))

section_visible.or(global_visible)
else
visible = visible.where(is_hidden: false)
# No section assigned - just check global visibility
visible = visible
.where(assessments: { visible_on: nil })
.or(visible.where(assessments: { visible_on: ..current_time }))
visible = visible
.where(assessments: { visible_until: nil })
.or(visible.where(assessments: { visible_until: current_time.. }))

# Use datetime if set, otherwise fall back to is_hidden
visible
.where.not(assessments: { visible_on: nil })
.or(visible.where.not(assessments: { visible_until: nil }))
.or(visible.where(assessments: { is_hidden: false }))
end
visible
end

def section_name
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class AddVisibilityDatesToAssessments < ActiveRecord::Migration[7.1]
def change
add_column :assessments, :visible_on, :datetime
add_column :assessments, :visible_until, :datetime
add_column :assessment_section_properties, :visible_on, :datetime
add_column :assessment_section_properties, :visible_until, :datetime
end
end
Loading