Skip to content

Conversation

@CK-7vn
Copy link
Member

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

Description of the change

This PR adds a feature to alert admins upon the attempted double booking of a resident in a class.

Screenshot(s)

image

Additional context

Also fixes a small bug with the pagination during Resident Enrollment caused by the following piece of code

  if (remainingCapacity < 0) {
      setErrorMessage('Class is full');
      setSelectedUsers(selectedUsers.slice(0, remainingCapacity));
  }
  if (remainingCapacity > 0 && errorMessage === 'Class is full') {
      setErrorMessage('');
  }

Flow of what happens and why it was causing issues with pagination:

  1. User clicks page 2 → setPageQuery(2) is called
  2. React starts rendering with new page number
  3. During render, the above code calls more state setters
  4. React detects state changes during render and bails out to prevent infinite loops
  5. The pagination UI updates (reads pageQuery directly), but the component doesn't fully re-render
  6. SWR doesn't refetch because the render cycle was interrupted
  7. Result: Page number changes but user list stays the same

Second issue:
the pagination button didn't have type="button" so when a user clicks a pagination page without any selected users it defaults to type="submit" causing "please select at least one user" in the lower left hand corner, and if you do have a user selected, enrolls that user.

@CK-7vn CK-7vn requested a review from a team as a code owner November 19, 2025 20:12
@CK-7vn CK-7vn requested review from calisio and removed request for a team November 19, 2025 20:12
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.

Looked good and tested good. Left a couple of comments.

@carddev81 carddev81 requested review from calisio, carddev81 and corypride and removed request for calisio December 6, 2025 16:02
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.

Looks good and tested good

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.

Nice work on the conflict detection - the logic is solid. Got a few thoughts that could improve the admin experience:

Issue: We're only showing the first conflicting class per user

There's a break statement on line 438 in backend/src/database/class_enrollments.go that stops checking after finding the first conflict. So if a resident's already enrolled in both "Math 101" and "English 102" that both conflict with the new class, admins only see one of them in the modal.

Example:

  • Resident enrolled in: Math 101 (Mon/Wed 2-3pm) and English 102 (Mon/Wed 2-3pm)
  • Trying to add: Science 3030 (Mon/Wed 2-3pm)
  • Current: Modal shows "Conflicts with Math 101"
  • Expected: Modal should show both Math 101 and English 102 conflicts
Image

Also was thinking about how to show that and maybe....

Bednar, Stephon
Conflicts with Math 101 · Mon 2:00 PM – 3:00 PM and at least one other class.

This gives admins awareness of other conflicting classes without cluttering the UI.

What do you think about these tweaks?

@CK-7vn
Copy link
Member Author

CK-7vn commented Dec 22, 2025

Nice work on the conflict detection - the logic is solid. Got a few thoughts that could improve the admin experience:

Issue: We're only showing the first conflicting class per user

There's a break statement on line 438 in backend/src/database/class_enrollments.go that stops checking after finding the first conflict. So if a resident's already enrolled in both "Math 101" and "English 102" that both conflict with the new class, admins only see one of them in the modal.

Example:

  • Resident enrolled in: Math 101 (Mon/Wed 2-3pm) and English 102 (Mon/Wed 2-3pm)
  • Trying to add: Science 3030 (Mon/Wed 2-3pm)
  • Current: Modal shows "Conflicts with Math 101"
  • Expected: Modal should show both Math 101 and English 102 conflicts
Image Also was thinking about how to show that and maybe....

Bednar, Stephon Conflicts with Math 101 · Mon 2:00 PM – 3:00 PM and at least one other class.

This gives admins awareness of other conflicting classes without cluttering the UI.

What do you think about these tweaks?

I'm not sure, in theory...there should never be two conflicts...if the double booking is working correctly, at least not both in the same exact time slots, If a user is in a class from 2-3pm, and then another class from 3-4pm, and then an admin trys adding them to a class between 2:30-3:30pm, then that's a possibility, I will look at the code again in the morning and see what the situation is for that, good catch.

@CK-7vn CK-7vn requested a review from corypride December 22, 2025 19:06
@CK-7vn CK-7vn force-pushed the CK-7vn/id-511 branch 2 times, most recently from be9b5c7 to 434a8d3 Compare December 22, 2025 20:07
@carddev81 carddev81 self-requested a review January 5, 2026 19:29
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.

Looks good.

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.

This tested good, but want to note that i did come across an issue where the conflicts didn't show up and I believe they should have, I was unable to recreate so I am approving this. Code looks great.

@carddev81 carddev81 merged commit 7c74532 into main Jan 6, 2026
10 checks passed
@carddev81 carddev81 deleted the CK-7vn/id-511 branch January 6, 2026 20: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.

5 participants