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.

Sign up for GitHub

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

Jump to bottom

Add non-hackers to /participants endpoint #366

Merged
merged 4 commits into from
Jan 26, 2024
Merged

Add non-hackers to /participants endpoint #366

merged 4 commits into from
Jan 26, 2024

Conversation

samderanova
Copy link
Contributor

Resolves #364.

Changes

  • Create a new function get_non_hackers in participant_manager to pull non-hackers from the database
    • The query for this function checks whether a role exists and if it does, whether that role is not an applicant or organizer role
  • Return concatenation of hackers and non-hackers in the function for the /admin/participants route

@samderanova samderanova linked an issue Jan 26, 2024 that may be closed by this pull request
[API] Provide non-hackers in participants list #364
Closed
@samderanova samderanova changed the title update: add non-hackers to /participants endpoint Add non-hackers to /participants endpoint Jan 26, 2024
@samderanova samderanova changed the title Add non-hackers to /participants endpoint Add non-hackers to /participants endpoint Jan 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 26, 2024 •
edited
Loading

Deploy preview for irvinehacks-site-2024 ready!

Name IrvineHacks 2024 Site
Preview Visit Preview
Commit deb0d8f

taesungh
taesungh reviewed Jan 26, 2024
View reviewed changes
Copy link
Member

@taesungh taesungh left a comment

Choose a reason for hiding this comment

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

Some interesting choices, think it could be simplified.

apps/api/src/routers/admin.py Outdated Show resolved Hide resolved
apps/api/src/admin/participant_manager.py Outdated Show resolved Hide resolved
apps/api/src/admin/participant_manager.py Outdated
Comment on lines 18 to 26
status: Union[Status, Decision]
status: Optional[Union[Status, Decision]] = None
Copy link
Member

Choose a reason for hiding this comment

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

Question: in what cases would a participant not have a status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to alter this to fit with non-hackers because there are some non-hackers who haven't signed the waiver yet and therefore don't have a status.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do you think you could manually assign a status to those who don't? Don't wanna mess too much with entire system right now, so maybe something like REVIEWED, even though it technically doesn't make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about WAIVER_UNSIGNED?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh never mind, that's not part of Status. I'll give them REVIEWED.

Copy link
Member

Choose a reason for hiding this comment

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

Oh i meant the database records, but I suppose this also technically works.

@samderanova samderanova force-pushed the update/participants-provide-non-hackers branch from edadd52 to fb46f06 Compare January 26, 2024 19:19
@samderanova samderanova force-pushed the update/participants-provide-non-hackers branch from fb46f06 to 78c0314 Compare January 26, 2024 19:21
taesungh
taesungh approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@taesungh taesungh left a comment

Choose a reason for hiding this comment

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

Approving to unblock, thanks for the patience with the rebase and merge conflict. Will have another conflict with #363.

@samderanova samderanova merged commit fbbc82d into main Jan 26, 2024
4 checks passed
@taesungh taesungh mentioned this pull request Jan 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taesungh taesungh taesungh approved these changes

@njhuey njhuey Awaiting requested review from njhuey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API] Provide non-hackers in participants list
2 participants
@samderanova @taesungh