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

Additional changes required to make fields optional #316

Merged
merged 18 commits into from
Jun 11, 2024
Merged

Conversation

Comeani
Copy link
Contributor

@Comeani Comeani commented Jun 6, 2024

I realized while working on getting the user creation from the admin account creation tool to work properly that the initial changes I made to the User model were not sufficient to actually have the requests be well defined supplying just the user name.

This PR allows for the fields to be blank in addition to no longer being required, and importantly adjusts the signatures for UserManager.create_user and UserManager.create_superuser so they are assumed to be in extra_fields instead of being positional arguments.

@Comeani
Copy link
Contributor Author

Comeani commented Jun 6, 2024

This will need to be in place for pitt-crc/sam_scripts#35 to work properly.

@Comeani Comeani requested a review from djperrefort June 6, 2024 22:55
Copy link
Member

@djperrefort djperrefort left a comment

Choose a reason for hiding this comment

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

@Comeani
Copy link
Contributor Author

Comeani commented Jun 7, 2024

I see, so blank is for validation in forms where null governs whether the database value can be null or an empty string.

So we want it to be OK to be null here so the requests with just the username are accepted, but blank should remain false as when it comes a user (or us on their behalf) making changes to their "user profile" values, first name/last name/email should be "required" fields before they can save any changes?

@Comeani Comeani requested a review from djperrefort June 7, 2024 19:06
@Comeani Comeani requested a review from djperrefort June 10, 2024 16:54
keystone_api/apps/users/managers.py Outdated Show resolved Hide resolved
@Comeani
Copy link
Contributor Author

Comeani commented Jun 10, 2024

@djperrefort So it turns out using str | None as the type for the inputs to create_user still caused them to be required positional arguments (at least when creating super users). Because the method for setting up the values passed into create_user when creating super users is deeper to django (and doesn't default to providing None as the argument), I think this is the simplest way to do it. The best alternative outright would be to just have my POSTs of user data include those other fields with an empty value, which I will probably need to do for passwords anyways.

@Comeani Comeani merged commit 66a5b93 into main Jun 11, 2024
13 checks passed
@Comeani Comeani deleted the optional_fields branch June 11, 2024 16:41
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