diff --git a/Makefile b/Makefile index a8afae32a0bd..773eecb2f7c4 100644 --- a/Makefile +++ b/Makefile @@ -154,6 +154,7 @@ ifneq ($(PR), false) endif initdb: + docker-compose run --rm web psql -h db -d postgres -U postgres -c "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname ='warehouse';" docker-compose run --rm web psql -h db -d postgres -U postgres -c "DROP DATABASE IF EXISTS warehouse" docker-compose run --rm web psql -h db -d postgres -U postgres -c "CREATE DATABASE warehouse ENCODING 'UTF8'" xz -d -f -k dev/$(DB).sql.xz --stdout | docker-compose run --rm web psql -h db -d warehouse -U postgres -v ON_ERROR_STOP=1 -1 -f - diff --git a/tests/common/db/base.py b/tests/common/db/base.py index 981110d96fbd..507d8a945c40 100644 --- a/tests/common/db/base.py +++ b/tests/common/db/base.py @@ -33,6 +33,7 @@ def _create(cls, *args, **kwargs): r = super()._create(*args, **kwargs) session = cls._meta.sqlalchemy_session session.flush() + session.expire_all() return r diff --git a/tests/unit/admin/views/test_projects.py b/tests/unit/admin/views/test_projects.py index 75ca271e3033..66635963a820 100644 --- a/tests/unit/admin/views/test_projects.py +++ b/tests/unit/admin/views/test_projects.py @@ -296,13 +296,13 @@ def test_invalid_key_query(self, db_request): reverse=True, ) db_request.matchdict["project_name"] = project.normalized_name - db_request.GET["q"] = "user:{}".format(journals[3].submitted_by) + db_request.GET["q"] = "user:username" result = views.journals_list(project, db_request) assert result == { "journals": journals[:25], "project": project, - "query": "user:{}".format(journals[3].submitted_by), + "query": "user:username", } def test_basic_query(self, db_request): diff --git a/tests/unit/legacy/api/test_simple.py b/tests/unit/legacy/api/test_simple.py index 8b3c2b87b1cf..89a80a441a8a 100644 --- a/tests/unit/legacy/api/test_simple.py +++ b/tests/unit/legacy/api/test_simple.py @@ -93,10 +93,6 @@ def test_no_files_with_serial(self, db_request): user = UserFactory.create() je = JournalEntryFactory.create(name=project.name, submitted_by=user) - # Make sure that we get any changes made since the JournalEntry was - # saved. - db_request.db.refresh(project) - assert simple.simple_detail(project, db_request) == { "project": project, "files": [], @@ -118,10 +114,6 @@ def test_with_files_no_serial(self, db_request): user = UserFactory.create() JournalEntryFactory.create(submitted_by=user) - # Make sure that we get any changes made since the JournalEntry was - # saved. - db_request.db.refresh(project) - assert simple.simple_detail(project, db_request) == { "project": project, "files": files, @@ -143,10 +135,6 @@ def test_with_files_with_serial(self, db_request): user = UserFactory.create() je = JournalEntryFactory.create(name=project.name, submitted_by=user) - # Make sure that we get any changes made since the JournalEntry was - # saved. - db_request.db.refresh(project) - assert simple.simple_detail(project, db_request) == { "project": project, "files": files, @@ -201,10 +189,6 @@ def test_with_files_with_version_multi_digit(self, db_request): user = UserFactory.create() je = JournalEntryFactory.create(name=project.name, submitted_by=user) - # Make sure that we get any changes made since the JournalEntry was - # saved. - db_request.db.refresh(project) - assert simple.simple_detail(project, db_request) == { "project": project, "files": files, diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index cbc3a777517d..4c4447f404fe 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -2217,20 +2217,41 @@ def test_manage_projects(self, db_request): newer_project_with_no_releases = ProjectFactory( releases=[], created=datetime.datetime(2018, 1, 1) ) - db_request.user = UserFactory( - projects=[ - project_with_older_release, - project_with_newer_release, - newer_project_with_no_releases, - older_project_with_no_releases, - ] + db_request.user = UserFactory() + RoleFactory.create( + user=db_request.user, + project=project_with_older_release, + role_name="Maintainer", + ) + RoleFactory.create( + user=db_request.user, project=project_with_newer_release, role_name="Owner" ) - user_second_owner = UserFactory( - projects=[project_with_older_release, older_project_with_no_releases] + RoleFactory.create( + user=db_request.user, + project=newer_project_with_no_releases, + role_name="Owner", + ) + RoleFactory.create( + user=db_request.user, + project=older_project_with_no_releases, + role_name="Maintainer", + ) + user_second_owner = UserFactory() + RoleFactory.create( + user=user_second_owner, + project=project_with_older_release, + role_name="Owner", + ) + RoleFactory.create( + user=user_second_owner, + project=older_project_with_no_releases, + role_name="Owner", + ) + RoleFactory.create( + user=user_second_owner, + project=project_with_newer_release, + role_name="Owner", ) - RoleFactory.create(user=db_request.user, project=project_with_newer_release) - RoleFactory.create(user=db_request.user, project=newer_project_with_no_releases) - RoleFactory.create(user=user_second_owner, project=project_with_newer_release) assert views.manage_projects(db_request) == { "projects": [ @@ -2329,30 +2350,15 @@ def test_delete_project_disallow_deletion(self): pretend.call("manage.project.settings", project_name="foo") ] - def test_get_project_contributors(self, db_request): - project = ProjectFactory.create(name="foo") - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - - db_request.user = UserFactory.create() - project.users = [db_request.user] - - res = views.get_project_contributors(project.name, db_request) - assert res == [db_request.user] - def test_get_user_role_in_project_single_role_owner(self, db_request): project = ProjectFactory.create(name="foo") db_request.session = pretend.stub( flash=pretend.call_recorder(lambda *a, **kw: None), ) db_request.user = UserFactory.create() - project.users = [db_request.user] - RoleFactory(user=db_request.user, project=project) + RoleFactory(user=db_request.user, project=project, role_name="Owner") - res = views.get_user_role_in_project( - project.name, db_request.user.username, db_request - ) + res = views.get_user_role_in_project(project, db_request.user, db_request) assert res == "Owner" def test_get_user_role_in_project_single_role_maintainer(self, db_request): @@ -2361,42 +2367,11 @@ def test_get_user_role_in_project_single_role_maintainer(self, db_request): flash=pretend.call_recorder(lambda *a, **kw: None), ) db_request.user = UserFactory.create() - project.users = [db_request.user] RoleFactory(user=db_request.user, project=project, role_name="Maintainer") - res = views.get_user_role_in_project( - project.name, db_request.user.username, db_request - ) + res = views.get_user_role_in_project(project, db_request.user, db_request) assert res == "Maintainer" - def test_get_user_role_in_project_two_roles_owner_and_maintainer(self, db_request): - project = ProjectFactory.create(name="foo") - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - db_request.user = UserFactory.create() - project.users = [db_request.user] - RoleFactory(user=db_request.user, project=project, role_name="Owner") - RoleFactory(user=db_request.user, project=project, role_name="Maintainer") - - res = views.get_user_role_in_project( - project.name, db_request.user.username, db_request - ) - assert res == "Owner" - - def test_get_user_role_in_project_no_role(self, db_request): - project = ProjectFactory.create(name="foo") - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None), - ) - db_request.user = UserFactory.create() - project.users = [db_request.user] - - res = views.get_user_role_in_project( - project.name, db_request.user.username, db_request - ) - assert res == "" - def test_delete_project(self, monkeypatch, db_request): project = ProjectFactory.create(name="foo") @@ -2407,16 +2382,13 @@ def test_delete_project(self, monkeypatch, db_request): db_request.POST["confirm_project_name"] = project.normalized_name db_request.user = UserFactory.create() + RoleFactory.create(project=project, user=db_request.user, role_name="Owner") + get_user_role_in_project = pretend.call_recorder( - lambda project_name, username, req: "Owner" + lambda project, user, req: "Owner" ) monkeypatch.setattr(views, "get_user_role_in_project", get_user_role_in_project) - get_project_contributors = pretend.call_recorder( - lambda project_name, req: [db_request.user] - ) - monkeypatch.setattr(views, "get_project_contributors", get_project_contributors) - send_removed_project_email = pretend.call_recorder(lambda req, user, **k: None) monkeypatch.setattr( views, "send_removed_project_email", send_removed_project_email @@ -2434,12 +2406,8 @@ def test_delete_project(self, monkeypatch, db_request): assert result.headers["Location"] == "/the-redirect" assert get_user_role_in_project.calls == [ - pretend.call(project.name, db_request.user.username, db_request,), - pretend.call(project.name, db_request.user.username, db_request,), - ] - - assert get_project_contributors.calls == [ - pretend.call(project.name, db_request,) + pretend.call(project, db_request.user, db_request,), + pretend.call(project, db_request.user, db_request,), ] assert send_removed_project_email.calls == [ @@ -2612,11 +2580,14 @@ def test_delete_project_release_disallow_deletion(self, monkeypatch): ] def test_delete_project_release(self, monkeypatch): + user = pretend.stub(username=pretend.stub()) release = pretend.stub( version="1.2.3", canonical_version="1.2.3", project=pretend.stub( - name="foobar", record_event=pretend.call_recorder(lambda *a, **kw: None) + name="foobar", + record_event=pretend.call_recorder(lambda *a, **kw: None), + users=[user], ), created=datetime.datetime(2017, 2, 5, 17, 18, 18, 462_634), ) @@ -2630,20 +2601,16 @@ def test_delete_project_release(self, monkeypatch): flags=pretend.stub(enabled=pretend.call_recorder(lambda *a: False)), route_path=pretend.call_recorder(lambda *a, **kw: "/the-redirect"), session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)), - user=pretend.stub(username=pretend.stub()), + user=user, remote_addr=pretend.stub(), ) journal_obj = pretend.stub() journal_cls = pretend.call_recorder(lambda **kw: journal_obj) get_user_role_in_project = pretend.call_recorder( - lambda project_name, username, req: "Owner" + lambda project, user, req: "Owner" ) monkeypatch.setattr(views, "get_user_role_in_project", get_user_role_in_project) - get_project_contributors = pretend.call_recorder( - lambda project_name, request: [request.user] - ) - monkeypatch.setattr(views, "get_project_contributors", get_project_contributors) monkeypatch.setattr(views, "JournalEntry", journal_cls) send_removed_project_release_email = pretend.call_recorder( @@ -2663,11 +2630,8 @@ def test_delete_project_release(self, monkeypatch): assert result.headers["Location"] == "/the-redirect" assert get_user_role_in_project.calls == [ - pretend.call(release.project.name, request.user.username, request,), - pretend.call(release.project.name, request.user.username, request,), - ] - assert get_project_contributors.calls == [ - pretend.call(release.project.name, request,) + pretend.call(release.project, request.user, request,), + pretend.call(release.project, request.user, request,), ] assert send_removed_project_release_email.calls == [ @@ -2821,6 +2785,7 @@ def test_delete_project_release_file(self, monkeypatch, db_request): release_file = FileFactory.create( release=release, filename=f"foobar-{release.version}.tar.gz" ) + RoleFactory.create(project=project, user=user) db_request.POST = { "confirm_project_name": release.project.name, @@ -2835,15 +2800,10 @@ def test_delete_project_release_file(self, monkeypatch, db_request): db_request.remote_addr = "1.2.3.4" get_user_role_in_project = pretend.call_recorder( - lambda project_name, username, req: "Owner" + lambda project, user, req: "Owner" ) monkeypatch.setattr(views, "get_user_role_in_project", get_user_role_in_project) - get_project_contributors = pretend.call_recorder( - lambda project_name, req: [db_request.user] - ) - monkeypatch.setattr(views, "get_project_contributors", get_project_contributors) - send_removed_project_release_file_email = pretend.call_recorder( lambda req, user, **k: None ) @@ -2885,12 +2845,8 @@ def test_delete_project_release_file(self, monkeypatch, db_request): ] assert get_user_role_in_project.calls == [ - pretend.call(project.name, db_request.user.username, db_request,), - pretend.call(project.name, db_request.user.username, db_request,), - ] - - assert get_project_contributors.calls == [ - pretend.call(project.name, db_request,) + pretend.call(project, db_request.user, db_request,), + pretend.call(project, db_request.user, db_request,), ] assert send_removed_project_release_file_email.calls == [ @@ -3043,7 +2999,7 @@ def test_get_manage_project_roles(self, db_request): ] assert result == { "project": project, - "roles_by_user": {user.username: [role]}, + "roles": {role}, "form": form_obj, } @@ -3071,7 +3027,7 @@ def test_post_new_role_validation_fails(self, db_request): assert form_obj.validate.calls == [pretend.call()] assert result == { "project": project, - "roles_by_user": {user.username: [role]}, + "roles": {role}, "form": form_obj, } @@ -3158,11 +3114,7 @@ def test_post_new_role(self, monkeypatch, db_request): assert result == { "project": project, - "roles_by_user": { - new_user.username: [role], - owner_1.username: [owner_1_role], - owner_2.username: [owner_2_role], - }, + "roles": {role, owner_1_role, owner_2_role}, "form": form_obj, } @@ -3219,7 +3171,7 @@ def test_post_duplicate_role(self, db_request): assert result == { "project": project, - "roles_by_user": {user.username: [role]}, + "roles": {role}, "form": form_obj, } @@ -3269,7 +3221,7 @@ def test_post_unverified_email(self, db_request, with_email): # No additional roles are created assert db_request.db.query(Role).all() == [] - assert result == {"project": project, "roles_by_user": {}, "form": form_obj} + assert result == {"project": project, "roles": set(), "form": form_obj} class TestChangeProjectRoles: @@ -3328,51 +3280,6 @@ def test_change_role_invalid_role_name(self, pyramid_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" - def test_change_role_when_multiple(self, db_request): - project = ProjectFactory.create(name="foobar") - user = UserFactory.create(username="testuser") - owner_role = RoleFactory.create(user=user, project=project, role_name="Owner") - maintainer_role = RoleFactory.create( - user=user, project=project, role_name="Maintainer" - ) - new_role_name = "Maintainer" - - db_request.method = "POST" - db_request.user = UserFactory.create() - db_request.remote_addr = "10.10.10.10" - db_request.POST = MultiDict( - [ - ("role_id", owner_role.id), - ("role_id", maintainer_role.id), - ("role_name", new_role_name), - ] - ) - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None) - ) - db_request.route_path = pretend.call_recorder(lambda *a, **kw: "/the-redirect") - - result = views.change_project_role(project, db_request) - - assert db_request.db.query(Role).all() == [maintainer_role] - assert db_request.route_path.calls == [ - pretend.call("manage.project.roles", project_name=project.name) - ] - assert db_request.session.flash.calls == [ - pretend.call("Changed role", queue="success") - ] - assert isinstance(result, HTTPSeeOther) - assert result.headers["Location"] == "/the-redirect" - - entry = ( - db_request.db.query(JournalEntry).options(joinedload("submitted_by")).one() - ) - - assert entry.name == project.name - assert entry.action == "remove Owner testuser" - assert entry.submitted_by == db_request.user - assert entry.submitted_from == db_request.remote_addr - def test_change_missing_role(self, db_request): project = ProjectFactory.create(name="foobar") missing_role_id = str(uuid.uuid4()) @@ -3414,36 +3321,6 @@ def test_change_own_owner_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" - def test_change_own_owner_role_when_multiple(self, db_request): - project = ProjectFactory.create(name="foobar") - user = UserFactory.create(username="testuser") - owner_role = RoleFactory.create(user=user, project=project, role_name="Owner") - maintainer_role = RoleFactory.create( - user=user, project=project, role_name="Maintainer" - ) - - db_request.method = "POST" - db_request.user = user - db_request.POST = MultiDict( - [ - ("role_id", owner_role.id), - ("role_id", maintainer_role.id), - ("role_name", "Maintainer"), - ] - ) - db_request.session = pretend.stub( - flash=pretend.call_recorder(lambda *a, **kw: None) - ) - db_request.route_path = pretend.call_recorder(lambda *a, **kw: "/the-redirect") - - result = views.change_project_role(project, db_request) - - assert db_request.session.flash.calls == [ - pretend.call("Cannot remove yourself as Owner", queue="error") - ] - assert isinstance(result, HTTPSeeOther) - assert result.headers["Location"] == "/the-redirect" - class TestDeleteProjectRoles: def test_delete_role(self, db_request): diff --git a/tests/unit/packaging/test_models.py b/tests/unit/packaging/test_models.py index 72bc59fe8c54..f4c318a12bef 100644 --- a/tests/unit/packaging/test_models.py +++ b/tests/unit/packaging/test_models.py @@ -28,14 +28,6 @@ ) -class TestRole: - def test_role_ordering(self, db_request): - project = DBProjectFactory.create() - owner_role = DBRoleFactory.create(project=project, role_name="Owner") - maintainer_role = DBRoleFactory.create(project=project, role_name="Maintainer") - assert max([maintainer_role, owner_role]) == owner_role - - class TestProjectFactory: @pytest.mark.parametrize(("name", "normalized"), [("foo", "foo"), ("Bar", "bar")]) def test_traversal_finds(self, db_request, name, normalized): diff --git a/tests/unit/packaging/test_views.py b/tests/unit/packaging/test_views.py index bebf8b3ae216..7b0966d3625b 100644 --- a/tests/unit/packaging/test_views.py +++ b/tests/unit/packaging/test_views.py @@ -174,9 +174,6 @@ def test_detail_rendered(self, db_request): for user in users: RoleFactory.create(user=user, project=project) - # Add an extra role for one user, to ensure deduplication - RoleFactory.create(user=users[0], project=project, role_name="another role") - result = views.release_detail(releases[1], db_request) assert result == { @@ -218,9 +215,6 @@ def test_detail_renders(self, monkeypatch, db_request): for user in users: RoleFactory.create(user=user, project=project) - # Add an extra role for one user, to ensure deduplication - RoleFactory.create(user=users[0], project=project, role_name="another role") - # patch the readme rendering logic. render_description = pretend.call_recorder( lambda raw, content_type: "rendered description" diff --git a/warehouse/locale/messages.pot b/warehouse/locale/messages.pot index a816a727c75a..d15db1f5a7b3 100644 --- a/warehouse/locale/messages.pot +++ b/warehouse/locale/messages.pot @@ -169,25 +169,25 @@ msgstr "" msgid "Email address ${email_address} verified. ${confirm_message}." msgstr "" -#: warehouse/manage/views.py:188 +#: warehouse/manage/views.py:186 msgid "Email ${email_address} added - check your email for a verification link" msgstr "" -#: warehouse/manage/views.py:669 warehouse/manage/views.py:705 +#: warehouse/manage/views.py:667 warehouse/manage/views.py:703 msgid "" "You must provision a two factor method before recovery codes can be " "generated" msgstr "" -#: warehouse/manage/views.py:680 +#: warehouse/manage/views.py:678 msgid "Recovery codes already generated" msgstr "" -#: warehouse/manage/views.py:681 +#: warehouse/manage/views.py:679 msgid "Generating new recovery codes will invalidate your existing codes." msgstr "" -#: warehouse/manage/views.py:731 +#: warehouse/manage/views.py:729 msgid "Invalid credentials. Try again" msgstr "" @@ -723,7 +723,7 @@ msgstr "" #: warehouse/templates/accounts/register.html:89 #: warehouse/templates/manage/account.html:270 #: warehouse/templates/manage/account/recovery_codes-provision.html:81 -#: warehouse/templates/manage/roles.html:140 +#: warehouse/templates/manage/roles.html:118 msgid "Username" msgstr "" @@ -747,8 +747,8 @@ msgstr "" #: warehouse/templates/manage/account.html:424 #: warehouse/templates/manage/account/totp-provision.html:69 #: warehouse/templates/manage/account/webauthn-provision.html:44 -#: warehouse/templates/manage/roles.html:137 -#: warehouse/templates/manage/roles.html:149 +#: warehouse/templates/manage/roles.html:115 +#: warehouse/templates/manage/roles.html:127 #: warehouse/templates/manage/token.html:129 #: warehouse/templates/manage/token.html:146 msgid "(required)" @@ -795,7 +795,7 @@ msgstr "" #: warehouse/templates/accounts/profile.html:23 #: warehouse/templates/manage/account.html:245 -#: warehouse/templates/manage/roles.html:56 +#: warehouse/templates/manage/roles.html:47 msgid "Avatar for {user} from gravatar.com" msgstr "" @@ -1864,7 +1864,7 @@ msgstr "" #: warehouse/templates/manage/account.html:462 #: warehouse/templates/manage/account.html:476 -#: warehouse/templates/manage/roles.html:121 +#: warehouse/templates/manage/roles.html:99 msgid "Remove" msgstr "" @@ -2333,7 +2333,7 @@ msgstr "" #: warehouse/templates/manage/journal.html:37 #: warehouse/templates/manage/journal.html:52 #: warehouse/templates/manage/roles.html:37 -#: warehouse/templates/manage/roles.html:135 +#: warehouse/templates/manage/roles.html:113 msgid "User" msgstr "" @@ -2684,8 +2684,8 @@ msgid "There are two possible roles for collaborators:" msgstr "" #: warehouse/templates/manage/roles.html:27 +#: warehouse/templates/manage/roles.html:64 #: warehouse/templates/manage/roles.html:73 -#: warehouse/templates/manage/roles.html:84 msgid "Maintainer" msgstr "" @@ -2697,8 +2697,8 @@ msgid "" msgstr "" #: warehouse/templates/manage/roles.html:29 -#: warehouse/templates/manage/roles.html:71 -#: warehouse/templates/manage/roles.html:84 +#: warehouse/templates/manage/roles.html:62 +#: warehouse/templates/manage/roles.html:73 msgid "Owner" msgstr "" @@ -2715,31 +2715,31 @@ msgid "Users who can manage %(project_name)s" msgstr "" #: warehouse/templates/manage/roles.html:38 -#: warehouse/templates/manage/roles.html:82 -#: warehouse/templates/manage/roles.html:147 +#: warehouse/templates/manage/roles.html:71 +#: warehouse/templates/manage/roles.html:125 msgid "Role" msgstr "" -#: warehouse/templates/manage/roles.html:90 +#: warehouse/templates/manage/roles.html:79 msgid "Save role" msgstr "" #: warehouse/templates/manage/account/recovery_codes-provision.html:65 -#: warehouse/templates/manage/roles.html:91 +#: warehouse/templates/manage/roles.html:80 msgid "Save" msgstr "" -#: warehouse/templates/manage/roles.html:115 +#: warehouse/templates/manage/roles.html:93 msgid "Cannot remove yourself as owner" msgstr "" -#: warehouse/templates/manage/roles.html:118 +#: warehouse/templates/manage/roles.html:96 #, python-format msgid "Remove %(user)s from this project" msgstr "" -#: warehouse/templates/manage/roles.html:130 -#: warehouse/templates/manage/roles.html:158 +#: warehouse/templates/manage/roles.html:108 +#: warehouse/templates/manage/roles.html:136 msgid "Add collaborator" msgstr "" diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 0c158440163f..e993fc555584 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -13,8 +13,6 @@ import base64 import io -from collections import defaultdict - import pyqrcode from paginate_sqlalchemy import SqlalchemyOrmPage as SQLAlchemyORMPage @@ -915,44 +913,13 @@ def manage_project_settings(project, request): return {"project": project} -def get_project_contributors(project_name, request): - query_res = ( - request.db.query(Project) - .join(User, Project.users) - .filter(Project.name == project_name) +def get_user_role_in_project(project, user, request): + return ( + request.db.query(Role) + .filter(Role.user == user, Role.project == project) .one() + .role_name ) - return query_res.users - - -def get_user_role_in_project(project_name, username, request): - raw_res = ( - request.db.query(Project) - .join(User, Project.users) - .filter(User.username == username, Project.name == project_name) - .with_entities(Role.role_name) - .distinct(Role.role_name) - .all() - ) - - query_res = [] - for el in raw_res: - if el.role_name is not None: - query_res.append(el) - - user_role = "" - # This check is needed because of - # issue https://github.com/pypa/warehouse/issues/2745 - # which is not yet resolved and a user could be an owner - # and a maintainer at the same time - if len(query_res) == 2 and ( - query_res[0].role_name == "Owner" or query_res[1].role_name == "Owner" - ): - user_role = "Owner" - if len(query_res) == 1: - user_role = query_res[0].role_name - - return user_role @view_config( @@ -978,15 +945,10 @@ def delete_project(project, request): confirm_project(project, request, fail_route="manage.project.settings") - submitter_role = get_user_role_in_project( - project.name, request.user.username, request - ) - contributors = get_project_contributors(project.name, request) + submitter_role = get_user_role_in_project(project, request.user, request) - for contributor in contributors: - contributor_role = get_user_role_in_project( - project.name, contributor.username, request - ) + for contributor in project.users: + contributor_role = get_user_role_in_project(project, contributor, request) send_removed_project_email( request, @@ -1130,9 +1092,8 @@ def delete_project_release(self): ) submitter_role = get_user_role_in_project( - self.release.project.name, self.request.user.username, self.request + self.release.project, self.request.user, self.request ) - contributors = get_project_contributors(self.release.project.name, self.request) self.request.db.add( JournalEntry( @@ -1159,9 +1120,9 @@ def delete_project_release(self): f"Deleted release {self.release.version!r}", queue="success" ) - for contributor in contributors: + for contributor in self.release.project.users: contributor_role = get_user_role_in_project( - self.release.project.name, contributor.username, self.request + self.release.project, contributor, self.request ) send_removed_project_release_email( @@ -1244,13 +1205,12 @@ def _error(message): ) submitter_role = get_user_role_in_project( - project_name, self.request.user.username, self.request + self.release.project, self.request.user, self.request ) - contributors = get_project_contributors(project_name, self.request) - for contributor in contributors: + for contributor in self.release.project.users: contributor_role = get_user_role_in_project( - project_name, contributor.username, self.request + self.release.project, contributor, self.request ) send_removed_project_release_file_email( @@ -1297,15 +1257,17 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): userid = user_service.find_userid(username) user = user_service.get_user(userid) - if request.db.query( + existing_role = ( request.db.query(Role) - .filter( - Role.user == user, Role.project == project, Role.role_name == role_name - ) - .exists() - ).scalar(): + .filter(Role.user == user, Role.project == project) + .first() + ) + if existing_role: request.session.flash( - f"User '{username}' already has {role_name} role for project", + ( + f"User '{username}' already has {existing_role.role_name} " + "role for project" + ), queue="error", ) elif user.primary_email is None or not user.primary_email.verified: @@ -1371,15 +1333,9 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): ) form = _form_class(user_service=user_service) - roles = request.db.query(Role).join(User).filter(Role.project == project).all() - - # TODO: The following lines are a hack to handle multiple roles for a - # single user and should be removed when fixing GH-2745 - roles_by_user = defaultdict(list) - for role in roles: - roles_by_user[role.user.username].append(role) + roles = set(request.db.query(Role).join(User).filter(Role.project == project).all()) - return {"project": project, "roles_by_user": roles_by_user, "form": form} + return {"project": project, "roles": roles, "form": form} @view_config( @@ -1391,95 +1347,43 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): has_translations=True, ) def change_project_role(project, request, _form_class=ChangeRoleForm): - # TODO: This view was modified to handle deleting multiple roles for a - # single user and should be updated when fixing GH-2745 - form = _form_class(request.POST) if form.validate(): - role_ids = request.POST.getall("role_id") - - if len(role_ids) > 1: - # This user has more than one role, so just delete all the ones - # that aren't what we want. - # - # TODO: This branch should be removed when fixing GH-2745. - roles = ( + role_id = request.POST["role_id"] + try: + role = ( request.db.query(Role) .join(User) - .filter( - Role.id.in_(role_ids), - Role.project == project, - Role.role_name != form.role_name.data, - ) - .all() - ) - removing_self = any( - role.role_name == "Owner" and role.user == request.user - for role in roles + .filter(Role.id == role_id, Role.project == project) + .one() ) - if removing_self: + if role.role_name == "Owner" and role.user == request.user: request.session.flash("Cannot remove yourself as Owner", queue="error") else: - for role in roles: - request.db.delete(role) - request.db.add( - JournalEntry( - name=project.name, - action=f"remove {role.role_name} {role.user.username}", - submitted_by=request.user, - submitted_from=request.remote_addr, - ) - ) - project.record_event( - tag="project:role:delete", - ip_address=request.remote_addr, - additional={ - "submitted_by": request.user.username, - "role_name": role.role_name, - "target_user": role.user.username, - }, - ) - request.session.flash("Changed role", queue="success") - else: - # This user only has one role, so get it and change the type. - try: - role = ( - request.db.query(Role) - .join(User) - .filter( - Role.id == request.POST.get("role_id"), Role.project == project + request.db.add( + JournalEntry( + name=project.name, + action="change {} {} to {}".format( + role.role_name, role.user.username, form.role_name.data + ), + submitted_by=request.user, + submitted_from=request.remote_addr, ) - .one() ) - if role.role_name == "Owner" and role.user == request.user: - request.session.flash( - "Cannot remove yourself as Owner", queue="error" - ) - else: - request.db.add( - JournalEntry( - name=project.name, - action="change {} {} to {}".format( - role.role_name, role.user.username, form.role_name.data - ), - submitted_by=request.user, - submitted_from=request.remote_addr, - ) - ) - role.role_name = form.role_name.data - project.record_event( - tag="project:role:change", - ip_address=request.remote_addr, - additional={ - "submitted_by": request.user.username, - "role_name": form.role_name.data, - "target_user": role.user.username, - }, - ) - request.session.flash("Changed role", queue="success") - except NoResultFound: - request.session.flash("Could not find role", queue="error") + role.role_name = form.role_name.data + project.record_event( + tag="project:role:change", + ip_address=request.remote_addr, + additional={ + "submitted_by": request.user.username, + "role_name": form.role_name.data, + "target_user": role.user.username, + }, + ) + request.session.flash("Changed role", queue="success") + except NoResultFound: + request.session.flash("Could not find role", queue="error") return HTTPSeeOther( request.route_path("manage.project.roles", project_name=project.name) @@ -1495,25 +1399,17 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): has_translations=True, ) def delete_project_role(project, request): - # TODO: This view was modified to handle deleting multiple roles for a - # single user and should be updated when fixing GH-2745 - - roles = ( - request.db.query(Role) - .join(User) - .filter(Role.id.in_(request.POST.getall("role_id")), Role.project == project) - .all() - ) - removing_self = any( - role.role_name == "Owner" and role.user == request.user for role in roles - ) - - if not roles: - request.session.flash("Could not find role", queue="error") - elif removing_self: - request.session.flash("Cannot remove yourself as Owner", queue="error") - else: - for role in roles: + try: + role = ( + request.db.query(Role) + .join(User) + .filter(Role.id == request.POST["role_id"]) + .one() + ) + removing_self = role.role_name == "Owner" and role.user == request.user + if removing_self: + request.session.flash("Cannot remove yourself as Owner", queue="error") + else: request.db.delete(role) request.db.add( JournalEntry( @@ -1532,7 +1428,9 @@ def delete_project_role(project, request): "target_user": role.user.username, }, ) - request.session.flash("Removed role", queue="success") + request.session.flash("Removed role", queue="success") + except NoResultFound: + request.session.flash("Could not find role", queue="error") return HTTPSeeOther( request.route_path("manage.project.roles", project_name=project.name) diff --git a/warehouse/migrations/versions/aaa60e8ea12e_enforce_uniqueness_of_user_id_project_.py b/warehouse/migrations/versions/aaa60e8ea12e_enforce_uniqueness_of_user_id_project_.py new file mode 100644 index 000000000000..346af88f4c27 --- /dev/null +++ b/warehouse/migrations/versions/aaa60e8ea12e_enforce_uniqueness_of_user_id_project_.py @@ -0,0 +1,49 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +enforce uniqueness of user_id, project_id on roles + +Revision ID: aaa60e8ea12e +Revises: 5c029d9ef925 +Create Date: 2020-03-04 21:56:32.651065 +""" + +from alembic import op + +revision = "aaa60e8ea12e" +down_revision = "5c029d9ef925" + + +def upgrade(): + op.execute( + """ + DELETE FROM roles + WHERE id IN ( + SELECT id FROM ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY project_id, user_id ORDER BY role_name DESC + ) as row_num + FROM roles + ) t + WHERE t.row_num > 1 + ) + RETURNING * + """ + ) + op.create_unique_constraint( + "_roles_user_project_uc", "roles", ["user_id", "project_id"] + ) + + +def downgrade(): + op.drop_constraint("_roles_user_project_uc", "roles", type_="unique") diff --git a/warehouse/packaging/models.py b/warehouse/packaging/models.py index 4efbc8e77cfa..19dc84dcd627 100644 --- a/warehouse/packaging/models.py +++ b/warehouse/packaging/models.py @@ -57,9 +57,12 @@ class Role(db.Model): __tablename__ = "roles" - __table_args__ = (Index("roles_user_id_idx", "user_id"),) + __table_args__ = ( + Index("roles_user_id_idx", "user_id"), + UniqueConstraint("user_id", "project_id", name="_roles_user_project_uc"), + ) - __repr__ = make_repr("role_name", "user_name", "package_name") + __repr__ = make_repr("role_name") role_name = Column(Text) user_id = Column( @@ -73,16 +76,6 @@ class Role(db.Model): user = orm.relationship(User, lazy=False) project = orm.relationship("Project", lazy=False) - def __gt__(self, other): - """ - Temporary hack to allow us to only display the 'highest' role when - there are multiple for a given user - - TODO: This should be removed when fixing GH-2745. - """ - order = ["Maintainer", "Owner"] # from lowest to highest - return order.index(self.role_name) > order.index(other.role_name) - class ProjectFactory: def __init__(self, request): diff --git a/warehouse/templates/manage/roles.html b/warehouse/templates/manage/roles.html index 8fcb6578d8d0..21a2c68711f7 100644 --- a/warehouse/templates/manage/roles.html +++ b/warehouse/templates/manage/roles.html @@ -40,16 +40,7 @@

{% trans %}Collaborators{% endtrans %}

- {# - The following two lines are added to handle multiple roles for a single - user. They should be removed when fixing GH-2745. - - TODO: Change them back to: - {% for role in roles|sort(attribute="user.username") %} - #} - {% for username, roles in roles_by_user|dictsort %} - {% set role = roles|max %} @@ -75,9 +66,7 @@

{% trans %}Collaborators{% endtrans %}

{% else %}
- {% for role in roles %} - - {% endfor %} + - {# - The following three lines are added to handle multiple roles for a - single user. They should be removed when fixing GH-2745. - - TODO: Change them back to: - - #} - {% for role in roles %} - - {% endfor %} -