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

Make officers module more DRY #94

Merged
merged 24 commits into from
Dec 29, 2024
Merged

Make officers module more DRY #94

merged 24 commits into from
Dec 29, 2024

Conversation

EarthenSky
Copy link
Collaborator

@EarthenSky EarthenSky commented Dec 9, 2024

Close #87

TODO:

  • implement and test the delete term endpoint
  • switch from datetime to date for dates
  • add all checks to OfficerInfoUpload.valid_or_raise & OfficerTermUpload.valid_or_raise
  • fix all TODOs in the module, or create issues for them
  • tests that all endpoints seem to work correctly (for GET)

see #102 for final changes

@micahdbak
Copy link
Collaborator

what's going on with the alembic upgrade head check? something with requirements? 😨

@EarthenSky
Copy link
Collaborator Author

alembic upgrade head check

I noticed it was failing but haven't looked into it yet. It's probably because I modified the existing alembic migration (changing from datetime -> date).

I updated the migration directly because I know we haven't deployed the database yet so it's easy to simply reset the db with the most up to date schema without losing any data. This is the last time we get to do that because I'm planning to add the officer data tonight :)

@micahdbak
Copy link
Collaborator

I updated the migration directly because I know we haven't deployed the database yet so it's easy to simply reset the db with the most up to date schema without losing any data. This is the last time we get to do that because I'm planning to add the officer data tonight :)

Hmm but the test runs on an empty db and fresh settings for alembic, which means it can't be created from scratch

Regardless I'd like to get this merged ASAP to make sure my FE works with the routes; I'm working on the election term updating feature RN, then next will be an admin page for adding new officers + terms

@micahdbak
Copy link
Collaborator

BTW, when you use the get officer info route, it returns discord ID, name, and username, but the patch route only takes name (as of what's in main); does ur PR fix that?

@EarthenSky
Copy link
Collaborator Author

Hmm but the test runs on an empty db ...

looks like it was due to the github action not our code; lucky!

BTW, when you use the get officer info route, it returns discord ID, name, and username ...

responded in discord!

@micahdbak micahdbak self-requested a review December 29, 2024 09:08
@micahdbak micahdbak marked this pull request as ready for review December 29, 2024 09:08
Copy link
Collaborator

@micahdbak micahdbak left a comment

Choose a reason for hiding this comment

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

🔥

@EarthenSky EarthenSky merged commit 8c2048f into main Dec 29, 2024
3 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.

Cleanup officers module
2 participants