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

add the login function #29

Closed
wants to merge 12 commits into from
Closed

add the login function #29

wants to merge 12 commits into from

Conversation

Chise1
Copy link

@Chise1 Chise1 commented Jan 27, 2022

I tried adding the login function. Including the following:

  1. A User model
  2. A script to create a user
  3. Reads the fields required for user-generated passwords from. Env.
  4. Login html can use i18n and Chinese.

Now there has some question:
1.I don't know how to create an executable with setup.py.(I can only use poetry to create a program by tool.poetry.scripts)
2. Most of this update is from my old project.But it need deeply test.
3. Doc need update.

In new version,you need create a .env in project's root.And .env need this data(like this):
SECRET_KEY=aawinjwol;egbfnjek bnl
DATABASE_URL=sqlite:///example.db

If you want to create a manager:
bash

sqladmin username password

@Chise1
Copy link
Author

Chise1 commented Jan 27, 2022

May be this project need use poetry to replace requirements.txt+setup.py

Copy link
Owner

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This is a great start.

) -> None:
self.app = app
self.engine = engine
self.base_url = base_url
self._model_admins: List[Type["ModelAdmin"]] = []

self.templates = Jinja2Templates("templates")
self.templates.env.add_extension("jinja2.ext.i18n")
if language:
translation = gettext.translation(
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather do this in a separate changes to keep small PR and make review easier. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I'll pay attention next time.

If you think I submit too many contents this time, I can submit them separately.

Copy link
Owner

Choose a reason for hiding this comment

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

that would be great. If you could focus this PR on the login functionality and do other PRs for translation and other things I'd really appreciate that.
PRs are always appreciated :)

@@ -204,7 +258,12 @@ async def list(self, request: Request) -> Response:

async def detail(self, request: Request) -> Response:
"""Detail route."""

if not check_token(request):
Copy link
Owner

Choose a reason for hiding this comment

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

A @login_required would be easier here.

sqladmin/auth/models.py Show resolved Hide resolved
@aminalaee
Copy link
Owner

May be this project need use poetry to replace requirements.txt+setup.py

Actually I use poetry in another project but I think this seems more reasonable. Why do you think so?

@Chise1
Copy link
Author

Chise1 commented Jan 27, 2022

I think poetry is easier and cheaper.

@Chise1
Copy link
Author

Chise1 commented Jan 27, 2022

I ran into a problem that the./script/build directive would run fine on my local, but not in CI/CD.

@aminalaee
Copy link
Owner

aminalaee commented Jan 27, 2022

I ran into a problem that the./script/build directive would run fine on my local, but not in CI/CD.

I'll fix that separately in another PR. Thanks

Update: I can't really fix that, it's the conf.py file you've added.

@Chise1
Copy link
Author

Chise1 commented Jan 28, 2022

I have cleared i18n code and add 'create manager' cli.

@aminalaee
Copy link
Owner

This generally looks good, but to be honest I think we should probably do something like Flask-Admin does here, and leave all user management, cli and things like that out of SQLAdmin. What do you think?
I don't have like a clear idea how to do that but if you feel like you want to do it, I think that approach seems reasonable.

@Chise1
Copy link
Author

Chise1 commented Jan 28, 2022

The Spring Festival is coming. I want to have a rest.

You're right, the question of whether you need to introduce authentication and permissions really needs to be thought about.

I'll get back to you over the holidays.

@Chise1
Copy link
Author

Chise1 commented Feb 9, 2022

Write

@Chise1 Chise1 closed this Feb 9, 2022
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.

None yet

2 participants