Fix several issues related to user management #406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gctucker
force-pushed
the
user-manager-fixup
branch
4 times, most recently
from
7463936 to
539cd7c
Compare
Convert self._email_sender to an attribute and only create the object when required. This way, the API can be run without email support as long as no endpoints that rely on email features are used. Signed-off-by: Guillaume Tucker <[email protected]>
Use create_user_manager() to get a UserManager object rather than constructing it directly in the API handlers. This removes dependencies in api.main, avoids duplication and simplifies the implementation. Signed-off-by: Guillaume Tucker <[email protected]>
Rework the UserManager constructor to call the parent class with all the arguments passed using the sandard idiom. The arguments aren't used in the constructor, so this makes it entirely generic, safer and future-proof. Signed-off-by: Guillaume Tucker <[email protected]>
Only look up if another user already exists if the update contains a username and it's not already the same one as the initial user. This avoids unnecessary errors when updating a user entry with the same username and avoids database lookups with a username set to None. Signed-off-by: Guillaume Tucker <[email protected]>
gctucker
force-pushed
the
user-manager-fixup
branch
from
539cd7c to
0d4d7cf
Compare
|
Tested OK locally. |
Closed
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes several issues found when testing the new user management implementation:
api.user_manager: call UserManager parent with all argumentsThis is to avoid unexpected behaviour and fix hidden bugs.
api.main: rely on create_user_manager()This is to have only one way of creating the UserManager objects and simplify the code
api.user_manager: create EmailSender only when requiredThis is to avoid requiring SMTP settings unless when actually trying to send an email
api.main: only look for duplicate user when requiredThis is to remove unwanted checks for duplicate users when no new username is being provided