-
Notifications
You must be signed in to change notification settings - Fork 642
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
Auth services #421
Auth services #421
Conversation
* Implements reset password and registration into stand alone services * Moves tokening into its own services * Adds basic user repository interface (this is provisional) * Adds support for tox
While I like these changes I am not sure what the ultimate goal of extracting these things into services is? Is this a step to make flaskbb integrate better with other flask apps?
I prefer to just have a single pair of those exceptions as we might end up with a lot more if we decide to migrate more things to this architecture.
I have recently moved everything to the setup.cfg file as I thought having everything in one place is better but I didn't know those tools deprecated the usage of the setup.cfg. |
flaskbb/auth/views.py
Outdated
from . import services | ||
from ..core.auth.password import ResetPasswordService | ||
from ..core.auth.registration import (RegistrationService, StopRegistration, | ||
UserRegistrationInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argh. I updated my formatter to use the pep8 style guide, I think I'll just go back to my old style setup and limit it to 79 chars.
flaskbb/core/auth/password.py
Outdated
:license: BSD, see LICENSE for more details | ||
""" | ||
|
||
from ..tokens import (StopTokenVerification, Token, TokenActions, TokenError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token
is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious that flake8 didn't yell about that.
from sqlalchemy import func | ||
|
||
from ...core.auth.registration import UserRegistrationError, UserValidator | ||
from ...core.tokens import TokenVerifier, TokenVerificationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports
I get the following error when trying to register:
|
That might be an effect, but it's to make building plugins easier and hopefully cut down on bugs as things will be focused down into small units. I'd like to build a JSON API plugin but in the current state of things, a lot of code would need to be duplicated over into it.
Will do. I felt silly doing it twice.
I found these issues:
Looks Flake8 still officially supports setup.cfg as a configuration source, but there is this issue filed against that tool that may or may not be happening: I agree cutting down on config files is a good thing, but using tox.ini for testing tools is probably the better one to standardize on since it seems like using setup.cfg can cause unintended side affects. We could probably move back into setup.cfg without many issues. |
I think we should keep this in mind when extracting these things into little services. This might make FlaskBB an interesting choice for small projects that want to have some kind of discussion forum.
I like this. The JSON API plugin would then serve as an example as well :) Thanks for the cleanups. |
Nope, this PR is already pretty big. I'd like to focus in on one or two services at a time lest this becomes a "I changed everything all at once" PR that no one will actually want to review in depth. |
While going through all the changes I wished that python 2 would support the |
Me too, me too. |
Two major changes here:
There are some concessions made in the APIs of these services, mostly in the view layer to cope with information that might not be available during install, tests, etc -- namely configuration settings coming from flaskbb_config. I figured doing a song and dance in the view layer will be easier to correct in the future than in the service layers.
Not all the logic has been extracted from the views and not all of it should be, but there are a few stray db.commits() that I'd like to move into the services but I haven't thought of a good way of handling this -- probably just doing it and paying the cost of another dependency on the services.
Some of it feels odd and a touch over engineered to me -- TokenVerificationError/StopTokenVerification and the mirrored UserRegistrationError/StopUserRegistrationError. We could probably merge these together into a single pair of ValidationError/StopValidation exceptions that could be reused across all areas.
There's also a newly added UserRepository, which I really like the idea of but I'm not sure how much it'll buy us and I might just chuck it honestly.
Other minor changes: