Skip to content

active field on User#2

Open
fallow64 wants to merge 14 commits intomainfrom
active_field
Open

active field on User#2
fallow64 wants to merge 14 commits intomainfrom
active_field

Conversation

@fallow64
Copy link

@fallow64 fallow64 commented Feb 6, 2026

This PR does the following:

  • Collapses active field into auth_netid and auth_username
  • Deletes username and password fields
  • Migrate Group.point_of_contact, Note.author, Project.staff1, and Project.staff2 to user IDs in the database
    • Return a UserMin object for those columns in the API

Also:

  • Fixes an issue where with_db_error_handling consumes any HTTP response exception
    • Also add logging of DB errors to with_db_error_handling
  • Fix all tests to pass
    • On a sidenote it'd be nice if the tests could be faster, I imagine there's probably some PyTest or FastAPI or SQLAlchemy options (specifically to avoid remaking the FastAPI app / reusing DB connections)

I'm slightly concerned about changing the behavior of with_db_error_handling incase anything wasn't meant to propagate to the user, but 🤷

@CannonLock CannonLock self-requested a review February 23, 2026 21:03
if not re.fullmatch(r'[^:,]*', username):
raise ValueError("Username cannot contain the characters ':' or ','.")
return username
class UserMin(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

Any reason that you did not use UserGet here?

I would prefer if you used UserGet. I know it transfers more bytes then needed but I like that it keeps the complexity down.


assert response.status_code == 400, f"Updating a group with a out of bounds unix_gid should return a 400 status code. Got {response.content} instead."

def test_add_group_with_point_of_contact(self, admin_client, user):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this same test for notes as well?

Copy link
Author

Choose a reason for hiding this comment

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

I have one for notes at test_user_foreign_keys.py:76, but it's definitely in a weird place (refactor?)

Copy link
Member

Choose a reason for hiding this comment

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

@fallow64 Oh great, yeah just copy paste to test_notes or if there is user specific stuff in it add a smaller test that is note specific.

if not re.fullmatch(r'[^:,]*', username):
raise ValueError("Username cannot contain the characters ':' or ','.")
return username
class UserMin(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

To reduce complexity can you use UserGet for this instead?

I am not worried about the increased transfer overhead.

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.

2 participants