From 2218b180a5a5eeafe23befccf3db05102b5a4a87 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 30 Nov 2018 11:06:49 -0500 Subject: [PATCH 1/4] do not show workspace role update alert when only environment role has changed --- atst/routes/workspaces/members.py | 14 ++++++-------- tests/routes/workspaces/test_members.py | 2 ++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/atst/routes/workspaces/members.py b/atst/routes/workspaces/members.py index 01be80e0..bfe85792 100644 --- a/atst/routes/workspaces/members.py +++ b/atst/routes/workspaces/members.py @@ -147,22 +147,20 @@ def update_member(workspace_id, member_id): form = EditMemberForm(http_request.form) if form.validate(): new_role_name = None - if form.data["workspace_role"] != member.role: + if form.data["workspace_role"] != member.role.name: member = Workspaces.update_member( g.current_user, workspace, member, form.data["workspace_role"] ) new_role_name = member.role_displayname + flash( + "workspace_role_updated", + member_name=member.user_name, + updated_role=new_role_name, + ) Environments.update_environment_roles( g.current_user, workspace, member, ids_and_roles ) - - flash( - "workspace_role_updated", - member_name=member.user_name, - updated_role=new_role_name, - ) - return redirect( url_for("workspaces.workspace_members", workspace_id=workspace.id) ) diff --git a/tests/routes/workspaces/test_members.py b/tests/routes/workspaces/test_members.py index 560d3934..47fe9325 100644 --- a/tests/routes/workspaces/test_members.py +++ b/tests/routes/workspaces/test_members.py @@ -93,6 +93,7 @@ def test_update_member_workspace_role(client, user_session): follow_redirects=True, ) assert response.status_code == 200 + assert b"role updated successfully" in response.data assert member.role_name == "security_auditor" @@ -140,6 +141,7 @@ def test_update_member_environment_role(client, user_session): follow_redirects=True, ) assert response.status_code == 200 + assert b"role updated successfully" not in response.data assert EnvironmentRoles.get(user.id, env1_id).role == "security_auditor" assert EnvironmentRoles.get(user.id, env2_id).role == "devops" From a75c19188f87c024992d0a1a833ff7c5b81b9f59 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 12 Dec 2018 11:39:58 -0500 Subject: [PATCH 2/4] display message for successfully updating environment role / access --- atst/domain/environments.py | 12 ++++++--- atst/routes/workspaces/members.py | 5 +++- atst/utils/flash.py | 5 ++++ tests/domain/test_environments.py | 33 +++++++++++++++++++++++-- tests/routes/workspaces/test_members.py | 2 ++ 5 files changed, 51 insertions(+), 6 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 38e23ff1..760d620a 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -65,6 +65,7 @@ class Environments(object): Permissions.ADD_AND_ASSIGN_CSP_ROLES, "assign environment roles", ) + updated = False for id_and_role in ids_and_roles: new_role = id_and_role["role"] @@ -72,19 +73,24 @@ class Environments(object): if new_role is None: EnvironmentRoles.delete(workspace_role.user.id, environment.id) + updated = True else: env_role = EnvironmentRoles.get( workspace_role.user.id, id_and_role["id"] ) - if env_role: + if env_role and env_role.role != new_role: env_role.role = new_role - else: + updated = True + db.session.add(env_role) + elif not env_role: env_role = EnvironmentRole( user=workspace_role.user, environment=environment, role=new_role ) - db.session.add(env_role) + updated = True + db.session.add(env_role) db.session.commit() + return updated @classmethod def revoke_access(cls, user, environment, target_user): diff --git a/atst/routes/workspaces/members.py b/atst/routes/workspaces/members.py index bfe85792..f3f06d4e 100644 --- a/atst/routes/workspaces/members.py +++ b/atst/routes/workspaces/members.py @@ -158,9 +158,12 @@ def update_member(workspace_id, member_id): updated_role=new_role_name, ) - Environments.update_environment_roles( + updated_roles = Environments.update_environment_roles( g.current_user, workspace, member, ids_and_roles ) + if updated_roles: + flash("environment_access_changed") + return redirect( url_for("workspaces.workspace_members", workspace_id=workspace.id) ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index 6196d2d2..5a6c8e90 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -96,6 +96,11 @@ MESSAGES = { """, "category": "warning", }, + "environment_access_changed": { + "title_template": "User access successfully changed.", + "message_template": "", + "category": "success", + }, } diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 25efe8cc..9f5e9aab 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -38,7 +38,7 @@ def test_update_environment_roles(): ] workspace_role = workspace.members[0] - Environments.update_environment_roles( + assert Environments.update_environment_roles( owner, workspace, workspace_role, new_ids_and_roles ) new_dev_env_role = EnvironmentRoles.get(workspace_role.user.id, dev_env.id) @@ -89,7 +89,7 @@ def test_remove_environment_role(): ] workspace_role = WorkspaceRoles.get(workspace.id, developer.id) - Environments.update_environment_roles( + assert Environments.update_environment_roles( owner, workspace, workspace_role, new_environment_roles ) @@ -99,6 +99,35 @@ def test_remove_environment_role(): assert EnvironmentRoles.get(developer.id, still_fa).role == "financial_auditor" +def test_no_update_to_environment_roles(): + owner = UserFactory.create() + developer = UserFactory.from_atat_role("developer") + + workspace = WorkspaceFactory.create( + owner=owner, + members=[{"user": developer, "role_name": "developer"}], + projects=[ + { + "name": "project1", + "environments": [ + { + "name": "project1 dev", + "members": [{"user": developer, "role_name": "devops"}], + } + ], + } + ], + ) + + dev_env = workspace.projects[0].environments[0] + new_ids_and_roles = [{"id": dev_env.id, "role": "devops"}] + + workspace_role = WorkspaceRoles.get(workspace.id, developer.id) + assert not Environments.update_environment_roles( + owner, workspace, workspace_role, new_ids_and_roles + ) + + def test_get_scoped_environments(db): developer = UserFactory.create() workspace = WorkspaceFactory.create( diff --git a/tests/routes/workspaces/test_members.py b/tests/routes/workspaces/test_members.py index 47fe9325..44a63879 100644 --- a/tests/routes/workspaces/test_members.py +++ b/tests/routes/workspaces/test_members.py @@ -142,6 +142,7 @@ def test_update_member_environment_role(client, user_session): ) assert response.status_code == 200 assert b"role updated successfully" not in response.data + assert b"access successfully changed" in response.data assert EnvironmentRoles.get(user.id, env1_id).role == "security_auditor" assert EnvironmentRoles.get(user.id, env2_id).role == "devops" @@ -169,6 +170,7 @@ def test_update_member_environment_role_with_no_data(client, user_session): follow_redirects=True, ) assert response.status_code == 200 + assert b"access successfully changed" not in response.data assert EnvironmentRoles.get(user.id, env1_id).role == "developer" From 8754a74613fcf27f226a47b9994c131cd88dd8b4 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 12 Dec 2018 11:57:54 -0500 Subject: [PATCH 3/4] environment role deletion method should also return a bool --- atst/domain/environment_roles.py | 3 +++ atst/domain/environments.py | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 5c0be864..18d056f1 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -21,3 +21,6 @@ class EnvironmentRoles(object): if existing_env_role: db.session.delete(existing_env_role) db.session.commit() + return True + else: + return False diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 760d620a..b3b42d9f 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -72,8 +72,11 @@ class Environments(object): environment = Environments.get(id_and_role["id"]) if new_role is None: - EnvironmentRoles.delete(workspace_role.user.id, environment.id) - updated = True + role_deleted = EnvironmentRoles.delete( + workspace_role.user.id, environment.id + ) + if role_deleted: + updated = True else: env_role = EnvironmentRoles.get( workspace_role.user.id, id_and_role["id"] From a5009ec1309cbcfa97799f988301f28e38b2e043 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 12 Dec 2018 13:09:48 -0500 Subject: [PATCH 4/4] do not commit environment role updates unless an update happened --- atst/domain/environments.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index b3b42d9f..b8da2282 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -92,7 +92,9 @@ class Environments(object): updated = True db.session.add(env_role) - db.session.commit() + if updated: + db.session.commit() + return updated @classmethod