Skip to content
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

Restructure user record to have multiple roles #429

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

taesungh
Copy link
Member

@taesungh taesungh commented Dec 6, 2024

Potentially controversial change to how user roles are managed. Expecting this will allow for more flexibility for new admin features and also given the need to support other types of applicants in #386.

Resolves #421. See description of changes for notable modifications besides renaming the field.

Changes

  • With the end goal of having more flexibility in assigning user roles, make UserRecord store multiple possible roles
    • Field is renamed to plural and modeled as a tuple or set
  • Provide empty array in default user identity instead of null
    • Semantically null could be better to indicate unknown rather than explicitly no roles, but functionally, an empty array is simpler
  • Constrain database query for check_in_participant
    • Still need unit tests for this
  • Update role and status check for confirm_attendance_non_hacker
  • Use AfterValidator for Applicant.roles to include Role.APPLICANT
  • Reorganize admin authorization roles to reuse dependencies
    • Basic admin access is provided to anybody with the "Organizer" role
    • Directors will need to have both "Organizer" and "Director" roles
  • Update logic for checking if user has already applied
  • Fix logical bug in ParticipantAction: function was used like boolean
  • Rename role strings to be the same as their display labels

Testing

Note any older records in the local database will need to be updated.

  1. Impersonate a new user and submit an application
  2. Observe in the database that the applicant has the correct default role: array with single entry of "Applicant"
  3. Add another sample user record with the "Organizer" role
  4. Impersonate said user and confirm the admin dashboard is accessible
  5. Add an application manager role, e.g. "Reviewer" to said user
  6. Observe the Applicants page of the admin site is accessible and the previously submitted application is included

@taesungh taesungh requested a review from a team December 6, 2024 20:09
- With the end goal of having more flexibility in assigning user roles,
  make `UserRecord` store multiple possible roles
  - Field is renamed to plural and modeled as a tuple or set
- Provide empty array in default user identity instead of `null`
  - Semantically `null` could be better to indicate unknown rather than
    explicitly no roles, but functionally, an empty array is simpler
- Constrain database query for `check_in_participant`
  - Still need unit tests for this
- Update role and status check for `confirm_attendance_non_hacker`
- Use `AfterValidator` for `Applicant.roles` to include `Role.APPLICANT`
- Reorganize admin authorization roles to reuse dependencies
  - Basic admin access is provided to anybody with the "Organizer" role
  - Directors will need to have both "Organizer" and "Director" roles
- Update logic for checking if user has already applied
- Fix logical bug in `ParticipantAction`: function was used like boolean
- Rename role strings to be the same as their display labels
@taesungh taesungh force-pushed the redesign/user-roles branch from a132e4b to 1331131 Compare December 6, 2024 20:16
Copy link
Contributor

github-actions bot commented Dec 6, 2024

Deploy preview for irvinehacks-site-2025 ready!

Name IrvineHacks Site
Preview Visit Preview
Commit 1331131

Copy link
Contributor

@waalbert waalbert left a comment

Choose a reason for hiding this comment

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

Thanks for this important user record change Taesung! This will allow us to have a lot more flexibility in giving roles.

@waalbert waalbert merged commit c47f95a into main Dec 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to have multiple roles
2 participants