Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
Binary file added .DS_Store
Binary file not shown.
Binary file added app/.DS_Store
Binary file not shown.
Binary file added app/models/.DS_Store
Binary file not shown.
11 changes: 7 additions & 4 deletions app/models/assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

class Assignment < ApplicationRecord
include MetricHelper
include DueDateActions
has_many :participants, class_name: 'AssignmentParticipant', foreign_key: 'parent_id', dependent: :destroy
has_many :users, through: :participants, inverse_of: :assignment
has_many :teams, class_name: 'AssignmentTeam', foreign_key: 'parent_id', dependent: :destroy, inverse_of: :assignment
Expand All @@ -11,8 +12,7 @@ class Assignment < ApplicationRecord
has_many :response_maps, foreign_key: 'reviewed_object_id', dependent: :destroy, inverse_of: :assignment
has_many :review_mappings, class_name: 'ReviewResponseMap', foreign_key: 'reviewed_object_id', dependent: :destroy, inverse_of: :assignment
has_many :sign_up_topics , class_name: 'SignUpTopic', foreign_key: 'assignment_id', dependent: :destroy
has_many :due_dates,as: :parent, class_name: 'DueDate', dependent: :destroy
has_many :due_dates,as: :parent, class_name: 'DueDate', dependent: :destroy
# Note: due_dates association is provided by DueDateActions mixin
belongs_to :course, optional: true
belongs_to :instructor, class_name: 'User', inverse_of: :assignments

Expand Down Expand Up @@ -118,17 +118,20 @@ def copy
# Save the copied assignment to the database
copied_assignment.save

# Copy all due dates to the new assignment
copy_due_dates_to(copied_assignment)

copied_assignment

end
def is_calibrated?
is_calibrated
end

def pair_programming_enabled?
enable_pair_programming
end

def has_badge?
has_badge
end
Expand Down
95 changes: 95 additions & 0 deletions app/models/concerns/due_date_actions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# frozen_string_literal: true

module DueDateActions
included do
has_many :due_dates, as: :parent, dependent: :destroy
end

# Generic activity permission checker that determines if an activity is permissible
# based on the current deadline state for this parent object
def activity_permissible?(activity)
current_deadline = next_due_date
return false unless current_deadline

current_deadline.activity_permissible?(activity)
end

# Activity permission checker for a specific deadline date (not current date)
def activity_permissible_for?(activity, deadline_date)
deadline = due_dates.where('due_at <= ?', deadline_date).order(:due_at).last
return false unless deadline

deadline.activity_permissible?(activity)
end

# Syntactic sugar methods for common activities
def submission_permissible?
activity_permissible?(:submission)
end

def review_permissible?
activity_permissible?(:review)
end

def teammate_review_permissible?
activity_permissible?(:teammate_review)
end

def quiz_permissible?
activity_permissible?(:quiz)
end

def team_formation_permissible?
activity_permissible?(:team_formation)
end

def signup_permissible?
activity_permissible?(:signup)
end

def drop_topic_permissible?
activity_permissible?(:drop_topic)
end

# Get the next due date for this parent
def next_due_date
due_dates.where('due_at >= ?', Time.current).order(:due_at).first
end

# Get the current stage name for display purposes
def current_stage
deadline = next_due_date
Copy link
Member

Choose a reason for hiding this comment

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

deadline --> upcoming_due_date

return 'finished' unless deadline

deadline.deadline_type&.name || 'unknown'
end

# Copy due dates to a new parent object
def copy_due_dates_to(new_parent)
due_dates.find_each do |due_date|
new_due_date = due_date.dup
new_due_date.parent = new_parent
new_due_date.save!
end
end

# Shift deadlines of a specific type by a time interval
def shift_deadlines_of_type(deadline_type_name, time_interval)
Copy link
Member

Choose a reason for hiding this comment

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

I would only expect the time interval to be expressed in DAYS. I think that using "days" as identifier names and in the calculation would improve the readability of this method.

due_dates.joins(:deadline_type)
.where(deadline_types: { name: deadline_type_name })
.update_all("due_at = due_at + INTERVAL #{time_interval.to_i} SECOND")
end

# Check if deadlines are in proper chronological order
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this method is useful. You can put them in order with a single statement. If you wanted them to be in order, you'd just sort them. Please remove this method.

def deadlines_properly_ordered?
sorted_deadlines = due_dates.order(:due_at)
previous_date = nil

sorted_deadlines.each do |deadline|
return false if previous_date && deadline.due_at < previous_date
previous_date = deadline.due_at
end

true
end
end
95 changes: 95 additions & 0 deletions app/models/concerns/due_date_permissions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# frozen_string_literal: true

module DueDatePermissions
# Permission checking methods that combine deadline-based and role-based logic

def can_submit?
Copy link
Member

Choose a reason for hiding this comment

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

Actually, these methods also need to check the can_submit, can_review, etc. fields of the Participant object for the currently logged in user. For example, if someone is only a reviewer for this assignment, and it's in the submission stage, this person cannot be allowed to submit because their Participant object will have can_submit = FALSE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that the permission logic for a participant exists in ParticipantsHelper, but I’m not sure where this module is supposed to get the actual participant object from. Should the permission methods receive a participant as an argument, or is there another place in the workflow where the current user’s participant is injected or made available?

return false unless submission_allowed_id

deadline_right = DeadlineRight.find_by(id: submission_allowed_id)
deadline_right&.name&.in?(%w[OK Late])
end

def can_review?
return false unless review_allowed_id

deadline_right = DeadlineRight.find_by(id: review_allowed_id)
deadline_right&.name&.in?(%w[OK Late])
end

def can_take_quiz?
return false unless quiz_allowed_id

deadline_right = DeadlineRight.find_by(id: quiz_allowed_id)
deadline_right&.name&.in?(%w[OK Late])
end

def can_teammate_review?
Copy link
Member

Choose a reason for hiding this comment

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

can_teammate_review? ==> can_review_teammate?

return false unless teammate_review_allowed_id

deadline_right = DeadlineRight.find_by(id: teammate_review_allowed_id)
deadline_right&.name&.in?(%w[OK Late])
end

# Generic permission checker
def activity_permissible?(activity)
permission_field = "#{activity}_allowed_id"
return false unless respond_to?(permission_field)

allowed_id = public_send(permission_field)
return false unless allowed_id

deadline_right = DeadlineRight.find_by(id: allowed_id)
deadline_right&.name&.in?(%w[OK Late])
end

# Check if deadline allows late submissions
def allows_late_submission?
return false unless submission_allowed_id

deadline_right = DeadlineRight.find_by(id: submission_allowed_id)
deadline_right&.name == 'Late'
end

def allows_late_review?
return false unless review_allowed_id

deadline_right = DeadlineRight.find_by(id: review_allowed_id)
deadline_right&.name == 'Late'
end

def allows_late_quiz?
return false unless quiz_allowed_id

deadline_right = DeadlineRight.find_by(id: quiz_allowed_id)
deadline_right&.name == 'Late'
end

# Check if this deadline is currently active (allows some action)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any need for this method. All deadlines allow for something to be done, in my experience. I also don't think it's necessary to check whether ANYTHING can be done ... just whether a specific action can be performed.

def active?
can_submit? || can_review? || can_take_quiz? || can_teammate_review?
end

# Get permission status for an action (OK, Late, No)
def permission_status_for(action)
permission_field = "#{action}_allowed_id"
return 'No' unless respond_to?(permission_field)

allowed_id = public_send(permission_field)
return 'No' unless allowed_id

deadline_right = DeadlineRight.find_by(id: allowed_id)
deadline_right&.name || 'No'
end

# Get a summary of all permissions for this deadline
Copy link
Member

Choose a reason for hiding this comment

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

Was not needed in DueDateActions, and is not needed here.

def permissions_summary
{
submission: permission_status_for(:submission),
review: permission_status_for(:review),
quiz: permission_status_for(:quiz),
teammate_review: permission_status_for(:teammate_review),
active: active?
Copy link
Member

Choose a reason for hiding this comment

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

Please remove. "active" is not helpful.

}
end
end
45 changes: 45 additions & 0 deletions app/models/concerns/due_date_queries.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# frozen_string_literal: true

module DueDateQueries
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a YAGNI class. I would remove it.

# Get next due date for this parent
Copy link
Member

Choose a reason for hiding this comment

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

I assume that "topic_id" here means the primary key of the topic in the topics table.
It is not guaranteed that all topics will have user-assigned numbers like "E2566".

def next_due_date(topic_id = nil)
if topic_id && has_topic_specific_deadlines?
topic_deadline = due_dates.where(parent_id: topic_id, parent_type: 'SignUpTopic')
Copy link
Member

Choose a reason for hiding this comment

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

"SignupTopics" (old repo) will be "ProjectTopics" in the new system.

.where('due_at >= ?', Time.current)
.order(:due_at)
.first
return topic_deadline if topic_deadline
end

due_dates.where('due_at >= ?', Time.current).order(:due_at).first
end

# Get all deadlines for a topic (topic-specific + assignment fallback)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any need to collect all deadlines for a topic. Another case of YAGNI.

def deadlines_for_topic(topic_id)
assignment_deadlines = due_dates.where(parent_type: 'Assignment')
topic_deadlines = due_dates.where(parent_id: topic_id, parent_type: 'SignUpTopic')

(assignment_deadlines + topic_deadlines).sort_by(&:due_at)
end

# Check if assignment has topic-specific deadlines
Copy link
Member

Choose a reason for hiding this comment

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

This check should really be made by checking whether the assignment is a staggered-deadline assignment. This is determined by the staggered_deadline field in the Assignment object.

def has_topic_specific_deadlines?
due_dates.where(parent_type: 'SignUpTopic').exists?
end

private

# Map action names to deadline type names for lookup
def action_to_deadline_type(action)
{
'submit' => 'submission',
'submission' => 'submission',
'review' => 'review',
'teammate_review' => 'teammate_review',
'quiz' => 'quiz',
'team_formation' => 'team_formation',
'signup' => 'signup',
'drop_topic' => 'drop_topic'
}[action.to_s.downcase]
end
end
51 changes: 51 additions & 0 deletions app/models/deadline_right.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# frozen_string_literal: true

class DeadlineRight < ApplicationRecord
validates :name, presence: true, uniqueness: true

# Class methods for finding deadline rights
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether the following methods would be needed. Please try to think of where they would be called. It's plausible that they would be helpful, but unless we can see a use, I think it's better not to include them.

def self.find_by_name(name)
find_by(name: name.to_s)
end

# Permission checking methods
def allows_action?
%w[OK Late].include?(name)
end

def denies_action?
name == 'No'
end

def allows_with_penalty?
name == 'Late'
end

def allows_without_penalty?
name == 'OK'
end

# Display methods
def to_s
name
end

def permission_level
case name
when 'No'
0
when 'Late'
1
when 'OK'
2
else
-1
end
end

def <=>(other)
return nil unless other.is_a?(DeadlineRight)

permission_level <=> other.permission_level
end
end
63 changes: 63 additions & 0 deletions app/models/deadline_type.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# frozen_string_literal: true

class DeadlineType < ApplicationRecord
validates :name, presence: true, uniqueness: true
validates :description, presence: true

has_many :due_dates, foreign_key: :deadline_type_id, dependent: :restrict_with_exception

# Class methods for finding deadline types
Copy link
Member

Choose a reason for hiding this comment

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

How would you envision these methods being used? If it's not clear, they should not be included.

def self.find_by_name(name)
find_by(name: name.to_s)
end

# Semantic helper methods for deadline type identification
def submission?
name == 'submission'
end

def review?
name == 'review'
end

def teammate_review?
name == 'teammate_review'
end

def quiz?
name == 'quiz'
end

def team_formation?
name == 'team_formation'
end

def signup?
name == 'signup'
end

def drop_topic?
name == 'drop_topic'
end

# Display methods
def display_name
name.humanize
end

def to_s
display_name
end

private

# Ensure we maintain referential integrity
def cannot_delete_if_has_due_dates
return unless due_dates.exists?

errors.add(:base, 'Cannot delete deadline type that has associated due dates')
throw :abort
end

before_destroy :cannot_delete_if_has_due_dates
end
Loading
Loading