E2566 Finish Due Dates #234

SeojinSeojin · 2025-11-25T01:14:10Z

No description provided.

github-actions · 2025-11-25T01:14:26Z

	2 Warnings
⚠️	Pull request is too big (more than 500 LoC).
⚠️	RSpec tests seem shallow (single `it` blocks or no context). Consider improving test structure.

Generated by 🚫 Danger

github-actions · 2025-11-25T01:17:48Z

🚨 RSpec Tests Report

Failing Model Test Cases:
rspec ./spec/models/due_date_spec.rb:13 # DueDate.fetch_due_dates fetches all the due_dates from a due_date's parent
rspec ./spec/models/due_date_spec.rb:33 # DueDate.sort_due_dates sorts a list of due dates from earliest to latest
rspec ./spec/models/due_date_spec.rb:52 # DueDate.any_future_due_dates? returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:63 # DueDate.any_future_due_dates? returns true when a no future due dates exist
rspec ./spec/models/due_date_spec.rb:78 # DueDate.set returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:96 # DueDate.copy copies the due dates from one assignment to another
rspec ./spec/models/due_date_spec.rb:135 # DueDate.next_due_date when parent_type is Assignment returns the next upcoming due date
rspec ./spec/models/due_date_spec.rb:168 # DueDate.next_due_date when parent_type is SignUpTopic calls TopicDueDate.next_due_date
rspec ./spec/models/due_date_spec.rb:174 # DueDate.next_due_date when parent_type is SignUpTopic returns the next upcoming due date for topics
rspec ./spec/models/due_date_spec.rb:179 # DueDate.next_due_date when parent_type is SignUpTopic returns the next assignment due date when topic has no upcoming due dates
rspec ./spec/models/due_date_spec.rb:190 # DueDate validation is invalid without a parent
rspec ./spec/models/due_date_spec.rb:196 # DueDate validation is invalid without a due_at
rspec ./spec/models/due_date_spec.rb:202 # DueDate validation is valid with required fields

efg

Please try to reduce the size of your code by 90%+. It would be much clearer if it was shorter and easy to grasp by just reading through it once.

efg · 2025-11-26T02:24:53Z

app/models/concerns/due_date_actions.rb

+    activity_permissible?(:drop_topic)
+  end
+
+  # Check activity permissions for a specific deadline type


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.

efg · 2025-11-26T02:26:16Z

app/models/concerns/due_date_actions.rb

+    current_deadline_for(action)
+  end
+
+  # Check if a specific action is currently allowed based on deadlines


This code seems repetitious. I wonder if there is a good way to simplify it, perhaps using metaprogramming. Be careful; ask an LLM.

efg · 2025-11-26T02:27:53Z

app/models/concerns/due_date_actions.rb

+    end
+  end
+
+  # Get all currently allowed actions


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.

efg · 2025-11-26T02:30:09Z

app/models/concerns/due_date_actions.rb

+    actions
+  end
+
+  # Check if any actions are currently allowed


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.

efg · 2025-11-26T02:32:30Z

app/models/concerns/due_date_actions.rb

+    allowed_actions.any?
+  end
+
+  # Get permission summary for all actions


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

efg · 2025-11-26T03:42:39Z

app/models/deadline_right.rb

+  scope :with_penalty, -> { where(name: 'Late') }
+  scope :without_penalty, -> { where(name: 'OK') }
+
+  # Class methods for finding deadline rights


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.

efg · 2025-11-26T03:43:16Z

app/models/deadline_right.rb

+    end
+  end
+
+  # Comparison methods


I'm pretty sure this would never be used.

efg · 2025-11-26T03:44:14Z

app/models/deadline_right.rb

+    permission_level <=> other.permission_level
+  end
+
+  # Method to seed the deadline rights (for use in migrations/seeds)


It seems to me that seeding code should not be in classes within the application. Probably more appropriate for test classes. Ask an LLM.

efg · 2025-11-26T03:44:55Z

app/models/deadline_right.rb

+    valid_right_names.include?(name.to_s)
+  end
+
+  # Statistics methods


YAGNI, I think.

efg · 2025-11-26T03:46:50Z

app/models/deadline_type.rb

+  scope :quiz_types, -> { where(name: ['quiz']) }
+  scope :administrative_types, -> { where(name: ['drop_topic', 'signup', 'team_formation']) }
+
+  # Class methods for finding deadline types


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

github-actions · 2025-11-26T05:30:21Z

🚨 RSpec Tests Report

All tests passed.

github-actions · 2025-11-26T05:30:41Z

🚨 RSpec Tests Report

All tests passed.

efg · 2025-11-26T20:59:27Z

app/models/concerns/due_date_actions.rb

-  # Shift deadlines of a specific type
-  def shift_deadlines_of_type(deadline_type_name, time_delta)
+  # Shift deadlines of a specific type by a time interval
+  def shift_deadlines_of_type(deadline_type_name, time_interval)


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.

efg · 2025-11-26T21:01:29Z

app/models/concerns/due_date_actions.rb

  end

-  # Check if deadlines are properly ordered (submission before review, etc.)
+  # Check if deadlines are in proper chronological order


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.

efg · 2025-11-26T21:03:51Z

app/models/concerns/due_date_permissions.rb

-    activities
-  end
-
  # Check if this deadline is currently active (allows some action)


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.

efg · 2025-11-26T21:04:24Z

app/models/concerns/due_date_permissions.rb

      teammate_review: permission_status_for(:teammate_review),
-      active: active?,
-      closed: closed?
+      active: active?


Please remove. "active" is not helpful.

efg · 2025-11-26T21:05:32Z

app/models/concerns/due_date_queries.rb

-                     .first
+  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')


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

efg · 2025-11-26T21:07:20Z

app/models/concerns/due_date_queries.rb

-  # Instance methods for parent objects (Assignment, SignUpTopic)
-  # These methods should be included in Assignment and SignUpTopic models
-
  # Get next due date for this parent


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".

efg · 2025-11-26T21:08:06Z

app/models/concerns/due_date_queries.rb

  end

-  # Get all deadlines affecting a specific topic
+  # Get all deadlines for a topic (topic-specific + assignment fallback)


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

efg · 2025-11-26T21:10:22Z

app/models/concerns/due_date_queries.rb


-  # Check if assignment has topic-specific overrides
-  def has_topic_deadline_overrides?
+  # Check if assignment has topic-specific deadlines


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.

github-actions · 2025-11-27T19:39:31Z

🚨 RSpec Tests Report

All tests passed.

github-actions · 2025-11-28T13:30:05Z

🚨 RSpec Tests Report

All tests passed.

github-actions · 2025-11-28T13:31:10Z

🚨 RSpec Tests Report

All tests passed.

Notes: talk about the participant parameter and anything else that is required before submission.

github-actions · 2025-11-28T21:49:00Z

🚨 RSpec Tests Report

Failing Model Test Cases:
rspec ./spec/models/due_date_spec.rb:13 # DueDate.fetch_due_dates fetches all the due_dates from a due_date's parent
rspec ./spec/models/due_date_spec.rb:33 # DueDate.sort_due_dates sorts a list of due dates from earliest to latest
rspec ./spec/models/due_date_spec.rb:52 # DueDate.any_future_due_dates? returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:63 # DueDate.any_future_due_dates? returns true when a no future due dates exist
rspec ./spec/models/due_date_spec.rb:78 # DueDate.set returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:96 # DueDate.copy copies the due dates from one assignment to another
rspec ./spec/models/due_date_spec.rb:135 # DueDate.next_due_date when parent_type is Assignment returns the next upcoming due date
rspec ./spec/models/due_date_spec.rb:168 # DueDate.next_due_date when parent_type is SignUpTopic calls TopicDueDate.next_due_date
rspec ./spec/models/due_date_spec.rb:174 # DueDate.next_due_date when parent_type is SignUpTopic returns the next upcoming due date for topics
rspec ./spec/models/due_date_spec.rb:179 # DueDate.next_due_date when parent_type is SignUpTopic returns the next assignment due date when topic has no upcoming due dates
rspec ./spec/models/due_date_spec.rb:190 # DueDate validation is invalid without a parent
rspec ./spec/models/due_date_spec.rb:196 # DueDate validation is invalid without a due_at
rspec ./spec/models/due_date_spec.rb:202 # DueDate validation is valid with required fields

github-actions · 2025-11-29T01:09:59Z

🚨 RSpec Tests Report

Failing Model Test Cases:
rspec ./spec/models/due_date_spec.rb:13 # DueDate.fetch_due_dates fetches all the due_dates from a due_date's parent
rspec ./spec/models/due_date_spec.rb:33 # DueDate.sort_due_dates sorts a list of due dates from earliest to latest
rspec ./spec/models/due_date_spec.rb:52 # DueDate.any_future_due_dates? returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:63 # DueDate.any_future_due_dates? returns true when a no future due dates exist
rspec ./spec/models/due_date_spec.rb:78 # DueDate.set returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:96 # DueDate.copy copies the due dates from one assignment to another
rspec ./spec/models/due_date_spec.rb:135 # DueDate.next_due_date when parent_type is Assignment returns the next upcoming due date
rspec ./spec/models/due_date_spec.rb:168 # DueDate.next_due_date when parent_type is SignUpTopic calls TopicDueDate.next_due_date
rspec ./spec/models/due_date_spec.rb:174 # DueDate.next_due_date when parent_type is SignUpTopic returns the next upcoming due date for topics
rspec ./spec/models/due_date_spec.rb:179 # DueDate.next_due_date when parent_type is SignUpTopic returns the next assignment due date when topic has no upcoming due dates
rspec ./spec/models/due_date_spec.rb:190 # DueDate validation is invalid without a parent
rspec ./spec/models/due_date_spec.rb:196 # DueDate validation is invalid without a due_at
rspec ./spec/models/due_date_spec.rb:202 # DueDate validation is valid with required fields

Failing Controller Test Cases:
rspec ./spec/requests/api/v1/assignment_controller_spec.rb:243 # Assignments API /assignments/{assignment_id}/copy_assignment post assignment copied successfully returns a 200 response

Refactored due date implementation. - Remove redundant next_due_date method from TopicDueDate class - Clean up DueDate class by removing duplicate class method - Improve next_due_date method in DueDateActions with better documentation - Add missing due_dates association to Assignment model (was incorrectly documented as provided by mixin) - Enhance DueDatePermissions documentation explaining participant permission requirements - Simplify TopicDueDate to proper single table inheritance subclass

github-actions · 2025-11-30T15:07:01Z

🚨 RSpec Tests Report

Failing Model Test Cases:
rspec ./spec/models/due_date_spec.rb:13 # DueDate.fetch_due_dates fetches all the due_dates from a due_date's parent
rspec ./spec/models/due_date_spec.rb:33 # DueDate.sort_due_dates sorts a list of due dates from earliest to latest
rspec ./spec/models/due_date_spec.rb:52 # DueDate.any_future_due_dates? returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:63 # DueDate.any_future_due_dates? returns true when a no future due dates exist
rspec ./spec/models/due_date_spec.rb:78 # DueDate.set returns true when a future due date exists
rspec ./spec/models/due_date_spec.rb:96 # DueDate.copy copies the due dates from one assignment to another
rspec ./spec/models/due_date_spec.rb:135 # DueDate.next_due_date when parent_type is Assignment returns the next upcoming due date
rspec ./spec/models/due_date_spec.rb:168 # DueDate.next_due_date when parent_type is SignUpTopic calls TopicDueDate.next_due_date
rspec ./spec/models/due_date_spec.rb:174 # DueDate.next_due_date when parent_type is SignUpTopic returns the next upcoming due date for topics
rspec ./spec/models/due_date_spec.rb:179 # DueDate.next_due_date when parent_type is SignUpTopic returns the next assignment due date when topic has no upcoming due dates
rspec ./spec/models/due_date_spec.rb:190 # DueDate validation is invalid without a parent
rspec ./spec/models/due_date_spec.rb:196 # DueDate validation is invalid without a due_at
rspec ./spec/models/due_date_spec.rb:202 # DueDate validation is valid with required fields

github-actions · 2025-12-01T03:18:46Z

🚨 RSpec Tests Report

All tests passed.

efg · 2025-12-01T03:50:13Z

app/models/concerns/due_date_actions.rb

  end

-  # Get the next due date for this parent
+  # Get the next due date for this assignment


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.

efg · 2025-12-01T03:51:50Z

app/models/due_date.rb

-
-      where(parent_id: parent_id).upcoming.first
-    end
+    # This method is no longer needed as the functionality has been moved


Agreed, but the method should be removed entirely from the code (comments and all).

…t_due_date

github-actions · 2025-12-01T17:25:25Z

🚨 RSpec Tests Report

All tests passed.

github-actions · 2025-12-02T00:01:30Z

🚨 RSpec Tests Report

All tests passed.

github-actions · 2025-12-02T19:14:51Z

🚨 RSpec Tests Report

Failing Model Test Cases:
rspec ./spec/models/assignment_spec.rb:183 # Assignment#deadlines_properly_ordered? returns false when ordering wrong

github-actions · 2025-12-02T19:17:13Z

🚨 RSpec Tests Report

All tests passed.

efg · 2025-12-03T14:22:59Z

app/models/concerns/due_date_actions.rb

+
+  # Get the current stage name for display purposes
+  def current_stage
+    deadline = next_due_date


deadline --> upcoming_due_date

…e_actions for more readability on line 15 and removed cannot_delete_if_has_due_dates as it would only be triggered via admin view which wouldn't exist anymore.

github-actions · 2025-12-03T15:07:44Z

🚨 RSpec Tests Report

All tests passed.

vihar2712 · 2025-12-16T01:12:40Z

db/schema.rb

+
+  create_table "deadline_types", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
+    t.string "name", null: false
+    t.text "description", null: false


do not need description text field. remove it

vihar2712 · 2025-12-16T01:12:50Z

db/schema.rb


+  create_table "deadline_rights", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
+    t.string "name", null: false
+    t.text "description", null: false


do not need description text field. remove it

vihar2712 · 2025-12-16T01:25:03Z

db/schema.rb

    t.integer "review_of_review_allowed_id"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
+    t.index ["deadline_type_id"], name: "fk_rails_ee95f82c0c"


get rid of description_url, flag, deadline_name

Pushing (Local) Changes to the Task1 and Task2 for Completion checks.

e3d03c9

SeojinSeojin mentioned this pull request Nov 25, 2025

E2566 finish due dates #233

Closed

efg reviewed Nov 26, 2025

View reviewed changes

MrScruffles added 2 commits November 26, 2025 00:26

Updates to comments in pullrequest

0049749

Update .DS_Store

e2e1540

efg reviewed Nov 26, 2025

View reviewed changes

fix: remove methods, revise shift deadlines type to use days

ec7a755

SeojinSeojin added 2 commits November 28, 2025 08:26

delete .ds_store, rename methods

9937b35

remove .ds_store

8f0fc6b

Updated based on comments.

29f4ba3

Notes: talk about the participant parameter and anything else that is required before submission.

removal of ActiveSupport::Concern (not needed)

096561b

SeojinSeojin added 2 commits November 30, 2025 22:14

fix: fix migration files for proper migration

40e9aef

test: update test codes for new due date modules

0c6907e

efg reviewed Dec 1, 2025

View reviewed changes

SeojinSeojin added 2 commits December 1, 2025 12:19

fix: code review - simplify comments, truncate chained method for nex…

c02ae6d

…t_due_date

fix: code review - remove comments for removed method

da53025

test: add test codes

58fbb12

SeojinSeojin added 2 commits December 2, 2025 14:11

test: add test codes for assignment model (with DueDateAction)

a5ca117

fix: fix deadlines_properly_ordered? method in due date actions mixin

d2a5841

efg reviewed Dec 3, 2025

View reviewed changes

Removal of Late methods as didn't' have use, added comment to due_dat…

9eb9a49

…e_actions for more readability on line 15 and removed cannot_delete_if_has_due_dates as it would only be triggered via admin view which wouldn't exist anymore.

vihar2712 reviewed Dec 16, 2025

View reviewed changes

E2566 Finish Due Dates #234

Are you sure you want to change the base?

E2566 Finish Due Dates #234

Uh oh!

Conversation

SeojinSeojin commented Nov 25, 2025

Uh oh!

github-actions bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 25, 2025

Uh oh!

efg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 29, 2025

Uh oh!

github-actions bot commented Nov 30, 2025

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

github-actions bot commented Dec 2, 2025

Uh oh!

github-actions bot commented Dec 2, 2025

Uh oh!

github-actions bot commented Dec 2, 2025

github-actions bot commented Nov 25, 2025 •

edited

Loading