-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/admin scope delete routes #315
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
base: release-0.9.1
Are you sure you want to change the base?
Feature/admin scope delete routes #315
Conversation
natkam
left a comment
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.
I'll review it tomorrow, but for now, I'd have two suggestions:
- Swap the order of if-else blocks where
if not <condition>: ... else: ..., to getif <condition>: ... else: ...instead. Conditions without negations are easier to read and understand. (Maybe in some places it makes sense to leave it this way, e.g. if checkingif not user.is_adminallows toreturnimmediately and omit theelsealtogether, but it seems to me that this is not always the case here.) - And can you please add some tests? (;
|
I see GitHub reports some linting/formatting issues - we use ruff as pre-commit hook to fix those. Thanks! |
|
Thanks, I'll adjust that. |
|
While writing the tests, I noticed that |
@BigFlagBurito what do you mean by "not enabled"? Do you use PostgreSQL or SQLite? I'm just about to take a closer look at the last database migration (seems like the user id is not autoincrementing), so I could try to fix other issues. |
|
I use SQLite. There, foreign_keys functionality must always be activated for every connection to the database. I am currently testing this code. @event.listens_for(Engine, "connect")
def _set_sqlite_pragma(dbapi_connection, connection_record):
if isinstance(dbapi_connection, sqlite3.Connection):
cursor = dbapi_connection.cursor()
cursor.execute("PRAGMA foreign_keys=ON;")
cursor.close()This does allow cascade delete to work. |
|
Hmmm I see that you have added some of the migration and database fixes which I have also implemented in another pull request. Sorry that you had to waste your time fixing those! Would it be acceptable for you to split this PR into two: one with the database-related fixes (the migration with named FKs and |
#303
New:
Scope.adminhas a functionality.refresh_user, a new attributeis_adminis set for the user.Extended:
DeleteStateis ignored for assignments.