-
Notifications
You must be signed in to change notification settings - Fork 172
E2566 Finish Due Dates #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
SeojinSeojin
wants to merge
17
commits into
expertiza:main
Choose a base branch
from
SeojinSeojin:e2566-duedate
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
e3d03c9
Pushing (Local) Changes to the Task1 and Task2 for Completion checks.
MrScruffles 0049749
Updates to comments in pullrequest
MrScruffles e2e1540
Update .DS_Store
MrScruffles ec7a755
fix: remove methods, revise shift deadlines type to use days
SeojinSeojin 9937b35
delete .ds_store, rename methods
SeojinSeojin 8f0fc6b
remove .ds_store
SeojinSeojin 29f4ba3
Updated based on comments.
MrScruffles 096561b
removal of ActiveSupport::Concern (not needed)
MrScruffles ab83d49
Final edits from the meeting we had on Friday.
MrScruffles 40e9aef
fix: fix migration files for proper migration
SeojinSeojin 0c6907e
test: update test codes for new due date modules
SeojinSeojin c02ae6d
fix: code review - simplify comments, truncate chained method for nex…
SeojinSeojin da53025
fix: code review - remove comments for removed method
SeojinSeojin 58fbb12
test: add test codes
SeojinSeojin a5ca117
test: add test codes for assignment model (with DueDateAction)
SeojinSeojin d2a5841
fix: fix deadlines_properly_ordered? method in due date actions mixin
SeojinSeojin 9eb9a49
Removal of Late methods as didn't' have use, added comment to due_dat…
MrScruffles File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module DueDateActions | ||
| # 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 | ||
| # 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 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 assignment | ||
| # | ||
| # This method abstracts away whether the assignment has topic-specific deadlines | ||
| # or assignment-level deadlines. The caller doesn't need to know the implementation | ||
| # details, they just ask for the next due date and get the appropriate one. | ||
| # | ||
| # @param topic_id [Integer, nil] Optional topic ID. If provided and the assignment | ||
| # has topic-specific deadlines, returns the next deadline for that topic. | ||
| # If the topic has no upcoming deadlines, falls back to assignment-level deadlines. | ||
| # @return [DueDate, nil] The next upcoming due date, or nil if none exist | ||
| def next_due_date(topic_id = nil) | ||
| # If a topic is specified and this assignment has topic-specific deadlines, | ||
| # look for topic due dates first | ||
| if topic_id && has_topic_specific_deadlines? | ||
| topic_deadline = due_dates.where(parent_id: topic_id, parent_type: 'ProjectTopic') | ||
| .upcoming | ||
| .first | ||
| return topic_deadline if topic_deadline | ||
| end | ||
|
|
||
| # Fall back to assignment-level due dates | ||
| # This handles both cases: | ||
| # 1. No topic specified | ||
| # 2. Topic specified but no topic-specific deadlines found | ||
| due_dates.upcoming.first | ||
| end | ||
|
|
||
| # Get the current stage name for display purposes | ||
| def current_stage | ||
| deadline = next_due_date() | ||
| return 'finished' unless deadline | ||
|
|
||
| deadline.deadline_type&.name || 'unknown' | ||
| end | ||
|
|
||
| # Check if assignment has topic-specific deadlines | ||
| def has_topic_specific_deadlines? | ||
| staggered_deadline || due_dates.where(parent_type: 'ProjectTopic').exists? | ||
| 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, days) | ||
| due_dates.joins(:deadline_type) | ||
| .where(deadline_types: { name: deadline_type_name }) | ||
| .update_all("due_at = due_at + INTERVAL #{days.to_i} DAY") | ||
| end | ||
|
|
||
| # Check if deadlines are in proper chronological order | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module DueDatePermissions | ||
| # Permission checking methods that combine deadline-based and role-based logic | ||
| # | ||
| # As Ed explained, these methods must check both: | ||
| # 1. Whether the action is permitted by the current deadline (submission_allowed_id, review_allowed_id, etc.) | ||
| # 2. Whether the participant has the necessary permissions (can_submit, can_review, can_take_quiz fields) | ||
| # | ||
| # The participant object represents how a user is participating in the assignment. | ||
| # Not all participants can do all actions - some might only submit, others only review, | ||
| # and others might only take quizzes. The can_* fields control these permissions. | ||
|
|
||
| # Check if submission is allowed based on both deadline and participant permissions | ||
| # @param participant [Participant, nil] The participant to check permissions for. | ||
| # If nil, only checks deadline-based permissions. | ||
| # @return [Boolean] true if submission is allowed, false otherwise | ||
| def can_submit?(participant = nil) | ||
| return false unless submission_allowed_id | ||
| return false if participant && !participant.can_submit | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: submission_allowed_id) | ||
| deadline_right&.name&.in?(%w[OK Late]) | ||
| end | ||
|
|
||
| # Check if review is allowed based on both deadline and participant permissions | ||
| # @param participant [Participant, nil] The participant to check permissions for. | ||
| # If nil, only checks deadline-based permissions. | ||
| # @return [Boolean] true if review is allowed, false otherwise | ||
| def can_review?(participant = nil) | ||
| return false unless review_allowed_id | ||
| return false if participant && !participant.can_review | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: review_allowed_id) | ||
| deadline_right&.name&.in?(%w[OK Late]) | ||
| end | ||
|
|
||
| # Check if taking a quiz is allowed based on both deadline and participant permissions | ||
| # @param participant [Participant, nil] The participant to check permissions for. | ||
| # If nil, only checks deadline-based permissions. | ||
| # @return [Boolean] true if taking quiz is allowed, false otherwise | ||
| def can_take_quiz?(participant = nil) | ||
| return false unless quiz_allowed_id | ||
| return false if participant && !participant.can_take_quiz | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: quiz_allowed_id) | ||
| deadline_right&.name&.in?(%w[OK Late]) | ||
| end | ||
|
|
||
| # Check if teammate review is allowed based on both deadline and participant permissions | ||
| # Note: teammate review uses the can_review permission field | ||
| # @param participant [Participant, nil] The participant to check permissions for. | ||
| # If nil, only checks deadline-based permissions. | ||
| # @return [Boolean] true if teammate review is allowed, false otherwise | ||
| def can_review_teammate?(participant = nil) | ||
| return false unless teammate_review_allowed_id | ||
| return false if participant && !participant.can_review | ||
|
|
||
| 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 | ||
SeojinSeojin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| def late_submission_allowed? | ||
| return false unless submission_allowed_id | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: submission_allowed_id) | ||
| deadline_right&.name == 'Late' | ||
| end | ||
|
|
||
| def late_review_allowed? | ||
| return false unless review_allowed_id | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: review_allowed_id) | ||
| deadline_right&.name == 'Late' | ||
| end | ||
|
|
||
| def late_quiz_allowed? | ||
| return false unless quiz_allowed_id | ||
|
|
||
| deadline_right = DeadlineRight.find_by(id: quiz_allowed_id) | ||
| deadline_right&.name == 'Late' | ||
| 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 | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class DeadlineRight < ApplicationRecord | ||
| validates :name, presence: true, uniqueness: true | ||
|
|
||
| # 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| # 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 | ||
|
|
||
| # 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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is just fine. I would suggest appending lines 66 & 67 to line 65 for better readability. I also think you may have gone overboard on the comments; perhaps you could ask an LLM how to comment the method just as clearly but more concisely.