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

Allow Users to be moderators and for field to be set via Admin dashboard #5249

Merged
merged 20 commits into from
Jan 24, 2019

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Jan 4, 2019

As part of address #4011 one task is:
Add a new field/migration to the User model, as well as corresponding UI in /admin for administrators to toggle this flag for users;

@crwilcox crwilcox changed the title Moderators Allow Users to be moderators and for field to be set via Admin dashboard Jan 4, 2019
@crwilcox
Copy link
Contributor Author

crwilcox commented Jan 4, 2019

I opened this early, my mistake, I am currently working on tests.

@crwilcox
Copy link
Contributor Author

crwilcox commented Jan 4, 2019

Tests have been run. Also verified the admin portal and setting/unsetting admin.

@crwilcox
Copy link
Contributor Author

crwilcox commented Jan 7, 2019

At this point the current state is that you can be a moderator and attempts to post to most all endpoints will fail. Yet to be done is to make the buttons disabled to make it clear what moderators can/can't do.

@di
Copy link
Member

di commented Jan 7, 2019

@crwilcox Mind saving that last commit for a separate PR? I was just about to leave some comments on the current status of this.

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

Comments on the commits before 16ceea9.

warehouse/accounts/__init__.py Show resolved Hide resolved
warehouse/accounts/interfaces.py Outdated Show resolved Hide resolved
docs/application.rst Outdated Show resolved Hide resolved
):

user = User(
username=username,
name=name,
password=self.hasher.hash(password),
is_active=is_active,
is_superuser=is_superuser,
is_superuser=False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing the defaults from the interface, I noticed that the default isn't respected with db.Model. This is possibly a bug. This is what you will see if you don't have these lines

self = <sqlalchemy.dialects.postgresql.psycopg2.PGDialect_psycopg2 object at 0x7f5f8020e668>, cursor = <cursor object at 0x7f5f7f5ec618; closed: -1>
statement = 'INSERT INTO users (username, name, password, is_active, disabled_for) VALUES (%(username)s, %(name)s, %(password)s, %(is_active)s, %(disabled_for)s) RETURNING users.id'
parameters = {'disabled_for': None, 'is_active': False, 'name': 'test_name', 'password': '$argon2i$v=19$m=1024,t=6,p=6$dG5NSWltzfn//3/vnbP23g$+7Alw6BCiPsC38K1JMkDmw', ...}
context = <sqlalchemy.dialects.postgresql.psycopg2.PGExecutionContext_psycopg2 object at 0x7f5f7ea9bd30>

    def do_execute(self, cursor, statement, parameters, context=None):
>       cursor.execute(statement, parameters)
E       sqlalchemy.exc.IntegrityError: (psycopg2.IntegrityError) null value in column "is_superuser" violates not-null constraint
E       DETAIL:  Failing row contains ($argon2i$v=19$m=1024,t=6,p=6$dG5NSWltzfn//3/vnbP23g$+7Alw6BCiPsC..., 2019-01-09 17:31:33.509108, null, test_user, test_name, f, 2019-01-09 17:31:33.509108, d, 2019-01-09 17:31:33.509108, b4bc1a89-aaf5-47a9-8b25-84a4f6c0d734, null, f).
E        [SQL: 'INSERT INTO users (username, name, password, is_active, disabled_for) VALUES (%(username)s, %(name)s, %(password)s, %(is_active)s, %(disabled_for)s) RETURNING users.id'] [parameters: {'username': 'test_user', 'name': 'test_name', 'password': '$argon2i$v=19$m=1024,t=6,p=6$dG5NSWltzfn//3/vnbP23g$+7Alw6BCiPsC38K1JMkDmw', 'is_active': False, 'disabled_for': None}] (Background on this error at: http://sqlalche.me/e/gkpj)

../lib/python3.6/site-packages/sqlalchemy/engine/default.py:509: IntegrityError

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that compare_server_default is off by default, so an auto-migration for this change wouldn't actually add the defaults. I fixed this in #5275.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @di I will rebase on my end and correct :)

@di
Copy link
Member

di commented Jan 16, 2019

@crwilcox Looks like the "nuke user" button on the User page needs to be conditionally disabled (or really, the whole "Actions" box can go away):

screen shot 2019-01-15 at 6 04 28 pm

@crwilcox
Copy link
Contributor Author

@di , latest push fixes that button to disable.

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