Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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.
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
300 changes: 300 additions & 0 deletions app/models/concerns/due_date_actions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,300 @@
# frozen_string_literal: true

module DueDateActions
extend ActiveSupport::Concern

included do
has_many :due_dates, as: :parent, dependent: :destroy
include DueDateQueries
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

# Syntactic sugar methods for common activities
# These provide clean, readable method names while avoiding DRY violations
def submission_permissible?
activity_permissible?(:submission)
end

def review_permissible?
activity_permissible?(:review)
end

def teammate_review_permissible?
activity_permissible?(:teammate_review)
end

def metareview_permissible?
activity_permissible?(:metareview)
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

# Check activity permissions for a specific deadline type
Copy link
Member

Choose a reason for hiding this comment

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

This method should be removed completely. The deadline type has nothing to do with whether an activity is permissible. Whether an activity is permissible is entirely determined by the submission_allowed_id, review_allowed_id, etc. Leaving this method in the code will simply confuse anyone who needs to maintain or extend this code.

def activity_permissible_for_type?(activity, deadline_type_name)
deadline = find_deadline(deadline_type_name)
return false unless deadline

deadline.activity_permissible?(activity)
end

# Get the current stage/deadline for a specific action
def current_stage_for(action)
current_deadline_for(action)
end

# Check if a specific action is currently allowed based on 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 code seems repetitious. I wonder if there is a good way to simplify it, perhaps using metaprogramming. Be careful; ask an LLM.

def action_allowed?(action)
case action.to_s.downcase
when 'submit', 'submission'
submission_permissible?
when 'review'
review_permissible?
when 'teammate_review'
teammate_review_permissible?
when 'metareview'
metareview_permissible?
when 'quiz'
quiz_permissible?
when 'team_formation'
team_formation_permissible?
when 'signup'
signup_permissible?
when 'drop_topic'
drop_topic_permissible?
else
false
end
end

# Get all currently allowed actions
Copy link
Member

Choose a reason for hiding this comment

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

I see no reason that this method would ever be needed. The currently allowed actions are determined by the ..._allowed fields in the next due date. Collecting them together serves no purpose that I can imagine.

def allowed_actions
actions = []
actions << 'submission' if submission_permissible?
actions << 'review' if review_permissible?
actions << 'teammate_review' if teammate_review_permissible?
actions << 'metareview' if metareview_permissible?
actions << 'quiz' if quiz_permissible?
actions << 'team_formation' if team_formation_permissible?
actions << 'signup' if signup_permissible?
actions << 'drop_topic' if drop_topic_permissible?
actions
end

# Check if any actions are currently allowed
Copy link
Member

Choose a reason for hiding this comment

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

Again, I can't imagine how this method would ever be useful. If an assignment is Finished (i.e., all of its due_dates are in the past), no actions are currently allowed. If the available_to_students bit is set and it has future due dates, some action is always allowed. I can think of no counterexamples in the history of Expertiza. Please remove it.

def has_allowed_actions?
allowed_actions.any?
end

# Get permission summary for all actions
Copy link
Member

Choose a reason for hiding this comment

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

Again, I can think of no use for this method. It should be removed.

def action_permissions_summary
{
submission: submission_permissible?,
review: review_permissible?,
teammate_review: teammate_review_permissible?,
metareview: metareview_permissible?,
quiz: quiz_permissible?,
team_formation: team_formation_permissible?,
signup: signup_permissible?,
drop_topic: drop_topic_permissible?,
has_any_permissions: has_allowed_actions?
}
end

# Topic-specific permission checking
Copy link
Member

Choose a reason for hiding this comment

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

I really think that these topic-specific methods are the wrong place to be checking permissions.
The (client) code that checks whether it is allowable to submit or review, etc. should not have to care about whether there are topic-specific deadlines. It is better to place those checks in the method that checks whether an action is allowed, i.e., in activity_permissible(). If you put the check there, it only needs to be put in one method in the system. You don't need separate ..._permissible_for_topic() methods. Thus, at line 14 above, you would check whether the assignment is a staggered-deadline assignment (e.g., myAssignment.staggered_deadline?) and if so, figure out which topic the current team has, and then look for what actions are permissible in advance of that topic deadline. This puts the check at ONE place in the code, and no other code needs to be aware of whether deadlines are based on topics.

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 that a good way to implement it would be (in pseudo-code)

if my_assignment.staggered_deadline? then current_deadline = next_due_date(our_topic)
else current_deadline = next_due_date.

def activity_permissible_for_topic?(activity, topic_id)
deadline = current_stage_for_topic(topic_id, activity)
return false unless deadline

deadline.activity_permissible?(activity)
end

# Topic-specific syntactic sugar methods
def submission_permissible_for_topic?(topic_id)
activity_permissible_for_topic?(:submission, topic_id)
end

def review_permissible_for_topic?(topic_id)
activity_permissible_for_topic?(:review, topic_id)
end

def quiz_permissible_for_topic?(topic_id)
activity_permissible_for_topic?(:quiz, topic_id)
end

# Copy all due dates to a new parent object
def copy_due_dates_to(new_parent)
due_dates.find_each do |due_date|
due_date.copy_to(new_parent)
end
end

# Duplicate due dates with modifications
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 method is too general. The only modification that we have in the system is to adjust the due dates by a certain number of days. The parameter could be the number of days later to make the copied deadlines (e.g., 364 days later); then the code would copy the deadlines exactly except for the due_at field, which would be adjusted.

def duplicate_due_dates_with_changes(new_parent, changes = {})
due_dates.map do |due_date|
due_date.duplicate_with_changes(changes.merge(parent: new_parent))
end
end

# Create a new due date for this parent
def create_due_date(deadline_type_name, due_at, round: 1, **attributes)
deadline_type = DeadlineType.find_by_name(deadline_type_name)
raise ArgumentError, "Invalid deadline type: #{deadline_type_name}" unless deadline_type

due_dates.create!(
deadline_type: deadline_type,
due_at: due_at,
round: round,
**attributes
)
end

# Update or create a due date for a specific type and round
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 confused about where this method would be needed. Is this the way you would change permissions? e.g., change review_allowed_id from "OK" to "no"? Wouldn't it be clearer to have simple getters and setters for the different fields in due_date?

def set_deadline(deadline_type_name, due_at, round: 1, **attributes)
deadline = find_deadline(deadline_type_name, round)

if deadline
deadline.update!(due_at: due_at, **attributes)
deadline
else
create_due_date(deadline_type_name, due_at, round: round, **attributes)
end
end

# Remove due dates of a specific type
Copy link
Member

Choose a reason for hiding this comment

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

I can see no use for this method. OTOH, it WOULD be useful to have a method to remove due dates for a specific ROUND. (Maybe the assignment had two rounds of review last semester, and you only want one round this semester.)

def remove_deadlines_of_type(deadline_type_name)
due_dates.joins(:deadline_type)
.where(deadline_types: { name: deadline_type_name })
.destroy_all
end

# Shift all deadlines by a certain amount of time
def shift_deadlines(time_delta)
due_dates.update_all("due_at = due_at + INTERVAL #{time_delta.to_i} SECOND")
end

# Shift deadlines of a specific type
Copy link
Member

Choose a reason for hiding this comment

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

No need for this method. Needlessly complicates the code. Remove it.

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

# Check if deadlines are properly ordered (submission before review, etc.)
Copy link
Member

Choose a reason for hiding this comment

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

Another method that is definitely not needed and must be removed.

def deadlines_properly_ordered?
workflow_deadlines = due_dates.joins(:deadline_type)
.where(deadline_types: { name: DeadlineType.workflow_order })
.order(:due_at)

previous_position = -1
workflow_deadlines.each do |deadline|
current_position = deadline.deadline_type.workflow_position
return false if current_position < previous_position
previous_position = current_position
end

true
end

# Get deadline ordering violations
Copy link
Member

Choose a reason for hiding this comment

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

No need for this. Too hard to understand. Remove it.

def deadline_ordering_violations
violations = []
workflow_deadlines = due_dates.joins(:deadline_type)
.where(deadline_types: { name: DeadlineType.workflow_order })
.order(:due_at)

workflow_deadlines.each_with_index do |deadline, index|
next_deadline = workflow_deadlines[index + 1]
next unless next_deadline

if deadline.deadline_type.workflow_position > next_deadline.deadline_type.workflow_position
violations << {
earlier_deadline: deadline,
later_deadline: next_deadline,
issue: "#{next_deadline.deadline_type_name} should come after #{deadline.deadline_type_name}"
}
end
end

violations
end

# Validate that all required deadline types are present
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 the difference between has_required_deadlines? and has_complete_deadline_schedule?. I severely doubt whether these methods are needed. In general, an instructor should be allowed to use whatever deadlines (s)he wants. If there's going to be no peer review (like with Program 1), it should not be necessary to have a review deadline. If Expertiza is being used just to evaluate teammate contributions, maybe you don't need a submission deadline, etc. My advice: Remove the next 3 methods.

def has_required_deadlines?(required_types = ['submission'])
required_types.all? { |type| has_deadline_type?(type) }
end

# Get missing required deadline types
def missing_required_deadlines(required_types = ['submission'])
required_types.reject { |type| has_deadline_type?(type) }
end

# Check if this object has a complete deadline schedule
def has_complete_deadline_schedule?
has_deadline_type?('submission') &&
(has_deadline_type?('review') || has_deadline_type?('quiz'))
end

# Get the workflow stage based on current time and deadlines
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 understand, for example, how the current deadline could be overdue. If it is past the deadline, then that deadline is no longer the current deadline. The method seems too confusing.

There may be a role for a method that returns the current stage. But we'd need to consider carefully what the states might be, and how to determine them. For starters, if the next deadline is submission or review, then the stage is submission or review. If there are no future deadlines, then the state would be "Finished" or "Complete". But I don't think "Pre-submission" makes sense.

def current_workflow_stage
current_deadline = next_due_date
return 'inactive' unless current_deadline

if current_deadline.overdue?
previous = current_deadline.previous_deadline
return previous ? previous.deadline_type_name : 'pre-submission'
else
current_deadline.deadline_type_name
end
end

# Check if object is in a specific workflow stage
def in_stage?(stage_name)
current_workflow_stage == stage_name
end

# Get all stages this object will go through
def workflow_stages
used_deadline_types.sort_by(&:workflow_position).map(&:name)
end

# Check if a stage has been completed
Copy link
Member

Choose a reason for hiding this comment

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

Another YAGNI method.

def stage_completed?(stage_name)
deadline = find_deadline(stage_name)
return false unless deadline

deadline.overdue?
end

# Get completion status for all stages
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

def stage_completion_status
workflow_stages.map do |stage|
{
stage: stage,
completed: stage_completed?(stage),
deadline: find_deadline(stage)
}
end
end
end
Loading
Loading