Skip to content

Conversation

@CK-7vn
Copy link
Member

@CK-7vn CK-7vn commented Nov 20, 2025

Description of the change

Adds categorized dropdown reasons for excused and unexcused when taking attendance.

Screenshot(s)

image

Additional context

Please include additional context or information that the reviewer needs to understand the PR. This includes:
Included a bug fix that occured in database/class_events.go where we were only searching for attendance that was linked to the latest schedule rule (event id) for the class.

  • If a class schedule was updated (e.g., time changed, days changed), a new "Event" (schedule rule) is often created.
  • Any attendance taken under the old schedule was linked to the old Event ID.
  • The query Where("event_id = ?", event.ID) was strictly filtering for the current schedule's ID, effectively hiding all attendance records attached to previous versions of the schedule.

So, i just fetched all the event IDs, first retrieve all of the event ids associated with said class_id, query by list, and expand the date range to filter the entirety of the requested month (time period)

@CK-7vn CK-7vn requested a review from a team as a code owner November 20, 2025 19:49
@CK-7vn CK-7vn requested review from carddev81 and removed request for a team November 20, 2025 19:49
@corypride corypride self-requested a review November 24, 2025 20:09
Copy link
Contributor

@corypride corypride left a comment

Choose a reason for hiding this comment

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

Works Well and the tests pass. Good job! Minor adjustment on: class_event.go and we are good. I approve your code, but not related to your PR, but something that copilot flagged are :

High Priority: Race Conditions (Data Loss Risk)
Functions affected:

handleAttendanceChange() - Lines ~200-220
handleReasonChange() - Lines ~225-240
handleNoteChange() - Lines ~195-200

Real-world scenario: User rapidly clicks status → selects reason → types note, and only the last change persists due to race conditions between the three update functions.

Medium Priority: Memory Leaks
Functions affected:

State initialization (lines 94-97) - modifiedRows never gets cleaned up
useEffect (lines 125-144) - Keeps accumulating data from different pages

Real-world scenario: User navigates through 10 pages with 20 users each = 200 entries in modifiedRows object that never get cleared, growing memory usage indefinitely. This gives your coworker concrete examples of exactly where the problems occur and what user behavior triggers them.

@CK-7vn
Copy link
Member Author

CK-7vn commented Nov 26, 2025

Works Well and the tests pass. Good job! Minor adjustment on: class_event.go and we are good. I approve your code, but not related to your PR, but something that copilot flagged are :

High Priority: Race Conditions (Data Loss Risk) Functions affected:

handleAttendanceChange() - Lines ~200-220 handleReasonChange() - Lines ~225-240 handleNoteChange() - Lines ~195-200

Real-world scenario: User rapidly clicks status → selects reason → types note, and only the last change persists due to race conditions between the three update functions.

  • React guarantees "prev" is the most recent committed state. Rapid sequential updates are actually batched but each sees the previous update's result. This is Reacts way of handling concurrent updates.

Medium Priority: Memory Leaks Functions affected:

State initialization (lines 94-97) - modifiedRows never gets cleaned up useEffect (lines 125-144) - Keeps accumulating data from different pages

Real-world scenario: User navigates through 10 pages with 20 users each = 200 entries in modifiedRows object that never get cleared, growing memory usage indefinitely. This gives your coworker concrete examples of exactly where the problems occur and what user behavior triggers them.

  • This is intentional state preservation, when the component unmounts it actually clears everything, so when a user navigates away, React unmounts and garbage collects all of the state. The pagination accumulation is intentiona in that if a user edits page 1, goes to page 2, edits there, then returns to apge 1, their page 1 edits should be guaranteed to be there, and the submit clears implicitly, On submit changes are posted, and the mutate() revalidates, the user navigates away, and the component unmounts, so it wont actually leak, it preserved unsaved edits across pagination until the user saves or actually navigates away.

Copy link
Contributor

@carddev81 carddev81 left a comment

Choose a reason for hiding this comment

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

@CK-7vn Tested good and everything persists. I did have a weird error occur that needs attention. When I first select 'Excused' or 'Unexcused' and don't make any selection leaving the default of 'Lockdown' the message 'is required' shows up. Fix this and the github conflicts and is good to go

image

@carddev81 carddev81 self-requested a review December 10, 2025 04:52
Copy link
Contributor

@carddev81 carddev81 left a comment

Choose a reason for hiding this comment

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

On edit of event attendances, when I select a different reason and click save it doesn't keep the changes, fix this and it should be ready to go

@CK-7vn CK-7vn requested a review from carddev81 December 10, 2025 17:31
Copy link
Contributor

@carddev81 carddev81 left a comment

Choose a reason for hiding this comment

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

Tested good

@carddev81 carddev81 merged commit 1c45e1c into main Dec 10, 2025
9 of 10 checks passed
@carddev81 carddev81 deleted the CK-7vn/id-507 branch December 10, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants