From 887c134b2535ba786ce50a9db0099b95121d1f11 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 13 Nov 2018 16:01:18 -0500 Subject: [PATCH 01/17] Add tests --- atst/models/workspace_role.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/models/workspace_role.py b/atst/models/workspace_role.py index 21d17a90..0f496503 100644 --- a/atst/models/workspace_role.py +++ b/atst/models/workspace_role.py @@ -57,6 +57,7 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): change_set["status"] = [from_status, to_status] return change_set + @property def event_details(self): return { From f21608ef5ec00f8437ac320476f5b2b140afce0f Mon Sep 17 00:00:00 2001 From: Montana Date: Wed, 14 Nov 2018 15:27:29 -0500 Subject: [PATCH 02/17] Listen for changes to environment roles and only log update events when something has changed --- atst/models/environment_role.py | 2 +- atst/models/mixins/auditable.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index b236617d..6239bb66 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -10,7 +10,7 @@ class CSPRole(Enum): NONSENSE_ROLE = "nonsense_role" -class EnvironmentRole(Base, mixins.TimestampsMixin): +class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): __tablename__ = "environment_roles" id = types.Id() diff --git a/atst/models/mixins/auditable.py b/atst/models/mixins/auditable.py index 8aee817a..2c77cc4c 100644 --- a/atst/models/mixins/auditable.py +++ b/atst/models/mixins/auditable.py @@ -52,7 +52,8 @@ class AuditableMixin(object): @staticmethod def audit_update(mapper, connection, target): - target.create_audit_event(connection, target, ACTION_UPDATE) + if AuditableMixin.get_history(target): + target.create_audit_event(connection, target, ACTION_UPDATE) def get_changes(self): """ From d13cf99b32c91c2145d51c2d7fc8e0dd9a2ef4c7 Mon Sep 17 00:00:00 2001 From: Montana Date: Wed, 14 Nov 2018 15:54:20 -0500 Subject: [PATCH 03/17] Add audit log attributes to environment role model --- atst/models/environment_role.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index 6239bb66..3ae37656 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -29,6 +29,29 @@ class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): self.role, self.user.full_name, self.environment.name, self.id ) + @property + def history(self): + previous_state = mixins.AuditableMixin.get_history(self) + auditable_previous_state = {} + if "role" in previous_state: + from_role = previous_state["role"] + to_role = self.role + auditable_previous_state["role"] = [from_role, to_role] + return auditable_previous_state + + @property + def event_details(self): + return { + "updated_user": self.user.displayname, + "updated_user_id": str(self.user_id), + "env": self.environment.displayname, + "env_id": str(self.environment_id), + "project": self.environment.project.name, + "project_id": str(self.environment.project_id), + "workspace": self.environment.project.workspace.name, + "workspace_id": str(self.environment.project.workspace.id), + } + Index( "environments_role_user_environment", From e5f183218cb3dd3d2c3fef1440fd478520a9454d Mon Sep 17 00:00:00 2001 From: Montana Date: Wed, 14 Nov 2018 16:09:08 -0500 Subject: [PATCH 04/17] Display audit log event with all information --- atst/models/environment_role.py | 4 ++-- templates/audit_log.html | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index 3ae37656..bbbcde5f 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -44,8 +44,8 @@ class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): return { "updated_user": self.user.displayname, "updated_user_id": str(self.user_id), - "env": self.environment.displayname, - "env_id": str(self.environment_id), + "environment": self.environment.displayname, + "environment_id": str(self.environment_id), "project": self.environment.project.name, "project_id": str(self.environment.project_id), "workspace": self.environment.project.workspace.name, diff --git a/templates/audit_log.html b/templates/audit_log.html index 5e679dfe..ba3eae10 100644 --- a/templates/audit_log.html +++ b/templates/audit_log.html @@ -33,9 +33,15 @@
{% endif %} - {% if event.changed_state %} - from {{ event.changed_state.role[0] }} to {{ event.changed_state.role[1] }} -
+ {% if event.event_details["environment"] %} +
+ in Environment {{ event.event_details["environment_id"] }} ({{ event.event_details["environment"] }}) +
+ in Project {{ event.event_details["project_id"] }} ({{ event.event_details["project"] }}) +
+ in Workspace {{ event.event_details["workspace_id"] }} ({{ event.event_details["workspace"] }}) + {% endif %} +
{% endif %} {% if event.workspace %} @@ -43,6 +49,11 @@ {% elif event.request %} on Request {{ event.request_id }} ({{ event.request.displayname }}) {% endif %} + + {% if event.changed_state %} + from {{ event.changed_state.role[0] }} to {{ event.changed_state.role[1] }} +
+ {% endif %} From e984d10ac26e361113473514d2ba43b01ae07b61 Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 26 Nov 2018 11:46:52 -0500 Subject: [PATCH 05/17] Fixup after rebase --- atst/models/environment_role.py | 12 ++++++------ atst/models/mixins/auditable.py | 7 ++----- templates/audit_log.html | 4 +--- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index bbbcde5f..da038324 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -31,18 +31,18 @@ class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def history(self): - previous_state = mixins.AuditableMixin.get_history(self) - auditable_previous_state = {} + previous_state = self.get_changes() + change_set = {} if "role" in previous_state: - from_role = previous_state["role"] + from_role = previous_state["role"][0] to_role = self.role - auditable_previous_state["role"] = [from_role, to_role] - return auditable_previous_state + change_set["role"] = [from_role, to_role] + return change_set @property def event_details(self): return { - "updated_user": self.user.displayname, + "updated_user_name": self.user.displayname, "updated_user_id": str(self.user_id), "environment": self.environment.displayname, "environment_id": str(self.environment_id), diff --git a/atst/models/mixins/auditable.py b/atst/models/mixins/auditable.py index 2c77cc4c..9d34dece 100644 --- a/atst/models/mixins/auditable.py +++ b/atst/models/mixins/auditable.py @@ -52,15 +52,12 @@ class AuditableMixin(object): @staticmethod def audit_update(mapper, connection, target): - if AuditableMixin.get_history(target): + if AuditableMixin.get_changes(target): target.create_audit_event(connection, target, ACTION_UPDATE) def get_changes(self): """ - This function borrows largely from a gist: - https://gist.github.com/ngse/c20058116b8044c65d3fbceda3fdf423#file-audit_mixin-py-L106-L120 - - It returns a dictionary of the form {item: [from_value, to_value]}, + This function returns a dictionary of the form {item: [from_value, to_value]}, where 'item' is the attribute on the target that has been updated, 'from_value' is the value of the attribute before it was updated, and 'to_value' is the current value of the attribute. diff --git a/templates/audit_log.html b/templates/audit_log.html index ba3eae10..b4b41daa 100644 --- a/templates/audit_log.html +++ b/templates/audit_log.html @@ -30,8 +30,6 @@ {% if event.event_details %} for User {{ event.event_details.updated_user_id }} ({{ event.event_details.updated_user_name }}) -
- {% endif %} {% if event.event_details["environment"] %}
@@ -50,7 +48,7 @@ on Request {{ event.request_id }} ({{ event.request.displayname }}) {% endif %} - {% if event.changed_state %} + {% if event.changed_state.role %} from {{ event.changed_state.role[0] }} to {{ event.changed_state.role[1] }}
{% endif %} From ce669fd08660d4459f990ffabd1f0444ca26a40e Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 26 Nov 2018 15:28:19 -0500 Subject: [PATCH 06/17] Add tests --- atst/models/workspace_role.py | 1 - tests/models/test_workspace_role.py | 52 +++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/atst/models/workspace_role.py b/atst/models/workspace_role.py index 0f496503..21d17a90 100644 --- a/atst/models/workspace_role.py +++ b/atst/models/workspace_role.py @@ -57,7 +57,6 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): change_set["status"] = [from_status, to_status] return change_set - @property def event_details(self): return { diff --git a/tests/models/test_workspace_role.py b/tests/models/test_workspace_role.py index d59b85cc..990c8e0b 100644 --- a/tests/models/test_workspace_role.py +++ b/tests/models/test_workspace_role.py @@ -13,10 +13,13 @@ from tests.factories import ( UserFactory, InvitationFactory, WorkspaceRoleFactory, + EnvironmentFactory, + EnvironmentRoleFactory, + ProjectFactory, ) -def test_has_no_history(session): +def test_has_no_ws_role_history(session): owner = UserFactory.create() user = UserFactory.create() @@ -33,7 +36,7 @@ def test_has_no_history(session): assert not create_event.changed_state -def test_has_role_history(session): +def test_has_ws_role_history(session): owner = UserFactory.create() user = UserFactory.create() @@ -58,7 +61,7 @@ def test_has_role_history(session): assert changed_events[0].changed_state["role"][1] == "admin" -def test_has_status_history(session): +def test_has_ws_status_history(session): owner = UserFactory.create() user = UserFactory.create() @@ -81,6 +84,49 @@ def test_has_status_history(session): assert changed_events[0].changed_state["status"][1] == "active" +def test_has_no_env_role_history(session): + owner = UserFactory.create() + user = UserFactory.create() + workspace = Workspaces.create(RequestFactory.create(creator=owner)) + project = ProjectFactory.create(workspace=workspace) + environment = EnvironmentFactory.create(project=project, name="new environment!") + + env_role = EnvironmentRoleFactory.create( + user=user, environment=environment, role="developer" + ) + create_event = ( + session.query(AuditEvent) + .filter(AuditEvent.resource_id == env_role.id, AuditEvent.action == "create") + .one() + ) + + assert not create_event.changed_state + + +def test_has_env_role_history(session): + owner = UserFactory.create() + user = UserFactory.create() + workspace = Workspaces.create(RequestFactory.create(creator=owner)) + workspace_role = WorkspaceRoleFactory.create(workspace=workspace, user=user) + project = ProjectFactory.create(workspace=workspace) + environment = EnvironmentFactory.create(project=project, name="new environment!") + + env_role = EnvironmentRoleFactory.create( + user=user, environment=environment, role="developer" + ) + Environments.update_environment_roles( + owner, workspace, workspace_role, [{"role": "admin", "id": environment.id}] + ) + changed_events = ( + session.query(AuditEvent) + .filter(AuditEvent.resource_id == env_role.id, AuditEvent.action == "update") + .all() + ) + # changed_state["role"] returns a list [previous role, current role] + assert changed_events[0].changed_state["role"][0] == "developer" + assert changed_events[0].changed_state["role"][1] == "admin" + + def test_event_details(): owner = UserFactory.create() user = UserFactory.create() From 9177d7f3e3bd1d3258971e1f724cc0b83d168f3f Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 26 Nov 2018 16:30:03 -0500 Subject: [PATCH 07/17] Handle update events when previous state was None --- atst/models/mixins/auditable.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/atst/models/mixins/auditable.py b/atst/models/mixins/auditable.py index 9d34dece..b39c8768 100644 --- a/atst/models/mixins/auditable.py +++ b/atst/models/mixins/auditable.py @@ -17,9 +17,12 @@ class AuditableMixin(object): request_id = resource.auditable_request_id() resource_type = resource.auditable_resource_type() display_name = resource.auditable_displayname() - changed_state = resource.auditable_changed_state() event_details = resource.auditable_event_details() + changed_state = ( + resource.auditable_changed_state() if action == ACTION_UPDATE else None + ) + audit_event = AuditEvent( user_id=user_id, workspace_id=workspace_id, @@ -69,7 +72,9 @@ class AuditableMixin(object): for attr in attrs: history = getattr(inspect(self).attrs, attr.key).history if history.has_changes(): - previous_state[attr.key] = [history.deleted.pop(), history.added.pop()] + deleted = history.deleted.pop() if history.deleted else None + added = history.added.pop() if history.added else None + previous_state[attr.key] = [deleted, added] return previous_state def auditable_changed_state(self): From 0615bcef89606e782f6b55443e06592b61b94810 Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 26 Nov 2018 16:30:47 -0500 Subject: [PATCH 08/17] Fix a test to work with new history attribute --- tests/domain/test_workspaces.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/domain/test_workspaces.py b/tests/domain/test_workspaces.py index efec7eb3..901089b0 100644 --- a/tests/domain/test_workspaces.py +++ b/tests/domain/test_workspaces.py @@ -134,7 +134,8 @@ def test_update_workspace_role_role(workspace, workspace_owner): "workspace_role": "developer", "dod_id": "1234567890", } - member = Workspaces.create_member(workspace_owner, workspace, user_data) + WorkspaceRoleFactory._meta.sqlalchemy_session_persistence = "flush" + member = WorkspaceRoleFactory.create(workspace=workspace) role_name = "admin" updated_member = Workspaces.update_member( From bcf5d2c1338ae937759fb27dd0e943442c16f1a6 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 27 Nov 2018 09:27:09 -0500 Subject: [PATCH 09/17] Remove Styleguide --- templates/navigation/global_navigation.html | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/templates/navigation/global_navigation.html b/templates/navigation/global_navigation.html index 245474cb..093b86bd 100644 --- a/templates/navigation/global_navigation.html +++ b/templates/navigation/global_navigation.html @@ -2,17 +2,6 @@
    - {% if g.dev %} - {{ SidenavItem("Styleguide", - href="/styleguide", - icon="visible", - active=g.matchesPath('/styleguide'), - subnav=[ - {"label":"Subnav 1", "href":"/styleguide?subnav1", "icon": "plus", "active": g.matchesPath('/styleguide?subnav1')}, - {"label":"Subnav 2", "href":"/styleguide?subnav2", "active": g.matchesPath('/styleguide?subnav2')}, - ]) }} - {% endif %} - {{ SidenavItem("Requests", href="/requests", icon="document", From fd6b0f9b2480b63daae708fbf7c6ee840a97bfc3 Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Tue, 27 Nov 2018 10:21:58 -0500 Subject: [PATCH 10/17] Add vue component for confirmation popover Have the confirmation popover in a separate Vue component fixes a bug in IE that was causing the `form` element in the popover to be ignored. Since `form`s cannot be nested, the `form` element in the popover was being discarded by IE and the revoke/resend invitation buttons did nothing. Breaking the functionality into a Vue component moves the `form` into a separate template. When the popover is displayed, the component is added to the DOM at the end, so the `form` is properly not-nested. --- .../confirmation_popover.test.js.snap | 3 ++ .../__tests__/confirmation_popover.test.js | 22 +++++++++++++ js/components/confirmation_popover.js | 32 +++++++++++++++++++ js/index.js | 2 ++ templates/components/confirmation_button.html | 27 ++++++---------- templates/workspaces/members/edit.html | 2 -- 6 files changed, 68 insertions(+), 20 deletions(-) create mode 100644 js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap create mode 100644 js/components/__tests__/confirmation_popover.test.js create mode 100644 js/components/confirmation_popover.js diff --git a/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap b/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap new file mode 100644 index 00000000..232dfb2f --- /dev/null +++ b/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap @@ -0,0 +1,3 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`ConfirmationPopover matches snapshot 1`] = ` `; diff --git a/js/components/__tests__/confirmation_popover.test.js b/js/components/__tests__/confirmation_popover.test.js new file mode 100644 index 00000000..d82564e5 --- /dev/null +++ b/js/components/__tests__/confirmation_popover.test.js @@ -0,0 +1,22 @@ +import { shallowMount } from '@vue/test-utils' + +import ConfirmationPopover from '../confirmation_popover' + + +describe('ConfirmationPopover', () => { + it('matches snapshot', () => { + const wrapper = shallowMount(ConfirmationPopover, { + propsData: { + action: '/some-url', + btn_text: 'Do something dangerous', + cancel_btn_text: 'Cancel', + confirm_btn_text: 'Confirm', + confirm_msg: 'Are you sure you want to do that?', + csrf_token: '42' + } + }) + + expect(wrapper).toMatchSnapshot() + }) +}) + diff --git a/js/components/confirmation_popover.js b/js/components/confirmation_popover.js new file mode 100644 index 00000000..5bccc719 --- /dev/null +++ b/js/components/confirmation_popover.js @@ -0,0 +1,32 @@ +export default { + name: 'confirmation-popover', + + props: { + action: String, + btn_text: String, + cancel_btn_text: String, + confirm_btn_text: String, + confirm_msg: String, + csrf_token: String + }, + + template: ` + + + + + ` +} diff --git a/js/index.js b/js/index.js index 4edd4c08..2d5b9ff2 100644 --- a/js/index.js +++ b/js/index.js @@ -23,6 +23,7 @@ import CcpoApproval from './components/forms/ccpo_approval' import MembersList from './components/forms/members_list' import LocalDatetime from './components/local_datetime' import RequestsList from './components/forms/requests_list' +import ConfirmationPopover from './components/confirmation_popover' Vue.config.productionTip = false @@ -50,6 +51,7 @@ const app = new Vue({ EditEnvironmentRole, EditProjectRoles, RequestsList, + ConfirmationPopover, }, mounted: function() { diff --git a/templates/components/confirmation_button.html b/templates/components/confirmation_button.html index 71b15a3b..31db3355 100644 --- a/templates/components/confirmation_button.html +++ b/templates/components/confirmation_button.html @@ -1,19 +1,10 @@ -{% macro ConfirmationButton(btn_text, action, csrf_token, confirm_msg="Are you sure?", confirm_btn="Confirm", cancel_btn="Cancel") -%} - - - - +{% macro ConfirmationButton(btn_text, action, confirm_msg="Are you sure?", confirm_btn="Confirm", cancel_btn="Cancel") -%} + + {%- endmacro %} diff --git a/templates/workspaces/members/edit.html b/templates/workspaces/members/edit.html index 0c1e8f7a..0b609e5b 100644 --- a/templates/workspaces/members/edit.html +++ b/templates/workspaces/members/edit.html @@ -44,14 +44,12 @@ {{ ConfirmationButton( "Revoke Invitation", url_for("workspaces.revoke_invitation", workspace_id=workspace.id, token=member.latest_invitation.token), - form.csrf_token ) }} {% endif %} {% if member.can_resend_invitation %} {{ ConfirmationButton ( "Resend Invitation", url_for("workspaces.resend_invitation", workspace_id=workspace.id, token=member.latest_invitation.token), - form.csrf_token, confirm_msg="Are you sure? This will send an email to invite the user to join this workspace." )}} {% endif %} From 178892c62e66b6127f49b0698f33df99a085a7ec Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Tue, 27 Nov 2018 11:00:51 -0500 Subject: [PATCH 11/17] Use localVue to squash missing custom element error --- .../__snapshots__/confirmation_popover.test.js.snap | 2 +- js/components/__tests__/confirmation_popover.test.js | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap b/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap index 232dfb2f..2146b1aa 100644 --- a/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap +++ b/js/components/__tests__/__snapshots__/confirmation_popover.test.js.snap @@ -1,3 +1,3 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`ConfirmationPopover matches snapshot 1`] = ` `; +exports[`ConfirmationPopover matches snapshot 1`] = ` `; diff --git a/js/components/__tests__/confirmation_popover.test.js b/js/components/__tests__/confirmation_popover.test.js index d82564e5..72369e00 100644 --- a/js/components/__tests__/confirmation_popover.test.js +++ b/js/components/__tests__/confirmation_popover.test.js @@ -1,11 +1,15 @@ -import { shallowMount } from '@vue/test-utils' +import { createLocalVue, shallowMount } from '@vue/test-utils' +import VTooltip from 'v-tooltip' import ConfirmationPopover from '../confirmation_popover' +const localVue = createLocalVue() +localVue.use(VTooltip) describe('ConfirmationPopover', () => { it('matches snapshot', () => { const wrapper = shallowMount(ConfirmationPopover, { + localVue, propsData: { action: '/some-url', btn_text: 'Do something dangerous', From 1feaa726d2beb119bf29cfe0cdc0d0bdab0fe43f Mon Sep 17 00:00:00 2001 From: Patrick Smith Date: Tue, 27 Nov 2018 11:13:23 -0500 Subject: [PATCH 12/17] Add another test to ensure csrf token added correctly --- .../__tests__/confirmation_popover.test.js | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/js/components/__tests__/confirmation_popover.test.js b/js/components/__tests__/confirmation_popover.test.js index 72369e00..cf31b253 100644 --- a/js/components/__tests__/confirmation_popover.test.js +++ b/js/components/__tests__/confirmation_popover.test.js @@ -7,20 +7,26 @@ const localVue = createLocalVue() localVue.use(VTooltip) describe('ConfirmationPopover', () => { - it('matches snapshot', () => { - const wrapper = shallowMount(ConfirmationPopover, { - localVue, - propsData: { - action: '/some-url', - btn_text: 'Do something dangerous', - cancel_btn_text: 'Cancel', - confirm_btn_text: 'Confirm', - confirm_msg: 'Are you sure you want to do that?', - csrf_token: '42' - } - }) + const wrapper = shallowMount(ConfirmationPopover, { + localVue, + propsData: { + action: '/some-url', + btn_text: 'Do something dangerous', + cancel_btn_text: 'Cancel', + confirm_btn_text: 'Confirm', + confirm_msg: 'Are you sure you want to do that?', + csrf_token: '42' + } + }) + it('matches snapshot', () => { expect(wrapper).toMatchSnapshot() }) + + it('renders form with hidden csrf input', () => { + const input = wrapper.find('input[type=hidden]') + expect(input.exists()).toBe(true) + expect(input.attributes('value')).toBe('42') + }) }) From 2105388f37d8f37c4b1f4f20d916564f101efb70 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 27 Nov 2018 14:48:23 -0500 Subject: [PATCH 13/17] Simplify env role history attribute --- atst/models/environment_role.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index da038324..6b46bef5 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -31,13 +31,7 @@ class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def history(self): - previous_state = self.get_changes() - change_set = {} - if "role" in previous_state: - from_role = previous_state["role"][0] - to_role = self.role - change_set["role"] = [from_role, to_role] - return change_set + return self.get_changes() @property def event_details(self): From 3c154f445c1c1a3dca561341044b3819fb6da9e8 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Sat, 17 Nov 2018 10:21:45 -0500 Subject: [PATCH 14/17] Send workspace invite email to email submitted in add new member form. --- atst/routes/workspaces.py | 2 +- tests/routes/test_workspaces.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/atst/routes/workspaces.py b/atst/routes/workspaces.py index c216d5ee..d691f878 100644 --- a/atst/routes/workspaces.py +++ b/atst/routes/workspaces.py @@ -264,7 +264,7 @@ def create_member(workspace_id): new_member = Workspaces.create_member(user, workspace, form.data) invite = Invitations.create(user, new_member) send_invite_email( - g.current_user.full_name, invite.token, new_member.user.email + g.current_user.full_name, invite.token, form.data["email"] ) return redirect( diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index 3737042e..1a91e723 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -1,5 +1,6 @@ import datetime from flask import url_for +import pytest from tests.factories import ( UserFactory, @@ -339,6 +340,29 @@ def test_existing_member_accepts_valid_invite(client, user_session): assert len(Workspaces.for_user(user)) == 1 +def test_existing_member_invite_sent_to_email_submitted_in_form( + client, user_session, queue +): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + member_form_data = { + "dod_id": user.dod_id, + "first_name": user.first_name, + "last_name": user.last_name, + "workspace_role": "developer", + "email": "example@example.com", + } + user_session(workspace.owner) + client.post( + url_for("workspaces.create_member", workspace_id=workspace.id), + data={**member_form_data}, + ) + + assert user.email != "example@example.com" + assert len(queue.get_queue().jobs[0].args[0]) == 1 + assert queue.get_queue().jobs[0].args[0][0] == "example@example.com" + + def test_new_member_accepts_valid_invite(monkeypatch, client, user_session): workspace = WorkspaceFactory.create() user_info = UserFactory.dictionary() From 385878e1ab853199ed25cd160c7a4ce834e0c37e Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Mon, 19 Nov 2018 17:21:35 -0500 Subject: [PATCH 15/17] Resend workspace invitations to email submitted in new member form. --- .../02d11579a581_add_email_to_invite.py | 50 +++++++++++++++++++ atst/domain/invitations.py | 7 ++- atst/models/invitation.py | 12 ++--- atst/routes/workspaces.py | 8 ++- tests/domain/test_invitations.py | 10 ++-- tests/factories.py | 1 + tests/routes/test_workspaces.py | 28 +++++++++++ 7 files changed, 97 insertions(+), 19 deletions(-) create mode 100644 alembic/versions/02d11579a581_add_email_to_invite.py diff --git a/alembic/versions/02d11579a581_add_email_to_invite.py b/alembic/versions/02d11579a581_add_email_to_invite.py new file mode 100644 index 00000000..3788c112 --- /dev/null +++ b/alembic/versions/02d11579a581_add_email_to_invite.py @@ -0,0 +1,50 @@ +"""Add email to invite + +Revision ID: 02d11579a581 +Revises: 4c0b8263d800 +Create Date: 2018-11-19 14:51:33.178358 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import text + + +# revision identifiers, used by Alembic. +revision = '02d11579a581' +down_revision = '4c0b8263d800' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('invitations', sa.Column('email', sa.String())) + conn = op.get_bind() + # Add non-null value to email column + conn.execute( + text( + """ + insert into invitations (email) + select u.email + from invitations i + inner join workspace_roles wr on i.workspace_role_id = wr.id + inner join users u on wr.user_id = u.id + where i.email is null; + """ + ) + ) + conn.execute( + text( + """ + update invitations + set email = 'example@example.com' + where email is null; + """ + ) + ) + op.alter_column('invitations', 'email', nullable=False) + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('invitations', 'email') + # ### end Alembic commands ### diff --git a/atst/domain/invitations.py b/atst/domain/invitations.py index 738525ab..5f69e039 100644 --- a/atst/domain/invitations.py +++ b/atst/domain/invitations.py @@ -54,13 +54,14 @@ class Invitations(object): return invite @classmethod - def create(cls, inviter, workspace_role): + def create(cls, inviter, workspace_role, email): invite = Invitation( workspace_role=workspace_role, inviter=inviter, user=workspace_role.user, status=InvitationStatus.PENDING, expiration_time=Invitations.current_expiration_time(), + email=email, ) db.session.add(invite) db.session.commit() @@ -120,4 +121,6 @@ class Invitations(object): previous_invitation = Invitations._get(token) Invitations._update_status(previous_invitation, InvitationStatus.REVOKED) - return Invitations.create(user, previous_invitation.workspace_role) + return Invitations.create( + user, previous_invitation.workspace_role, previous_invitation.email + ) diff --git a/atst/models/invitation.py b/atst/models/invitation.py index 3768849c..6097361a 100644 --- a/atst/models/invitation.py +++ b/atst/models/invitation.py @@ -42,11 +42,13 @@ class Invitation(Base, TimestampsMixin, AuditableMixin): expiration_time = Column(TIMESTAMP(timezone=True)) - token = Column(String(), index=True, default=lambda: secrets.token_urlsafe()) + token = Column(String, index=True, default=lambda: secrets.token_urlsafe()) + + email = Column(String, nullable=False) def __repr__(self): - return "".format( - self.user_id, self.workspace_role_id, self.id + return "".format( + self.user_id, self.workspace_role_id, self.id, self.email ) @property @@ -82,10 +84,6 @@ class Invitation(Base, TimestampsMixin, AuditableMixin): if self.workspace_role: return self.workspace_role.workspace - @property - def user_email(self): - return self.workspace_role.user.email - @property def user_name(self): return self.workspace_role.user.full_name diff --git a/atst/routes/workspaces.py b/atst/routes/workspaces.py index d691f878..c0a0cab6 100644 --- a/atst/routes/workspaces.py +++ b/atst/routes/workspaces.py @@ -262,10 +262,8 @@ def create_member(workspace_id): if form.validate(): try: new_member = Workspaces.create_member(user, workspace, form.data) - invite = Invitations.create(user, new_member) - send_invite_email( - g.current_user.full_name, invite.token, form.data["email"] - ) + invite = Invitations.create(user, new_member, form.data["email"]) + send_invite_email(g.current_user.full_name, invite.token, invite.email) return redirect( url_for( @@ -381,7 +379,7 @@ def revoke_invitation(workspace_id, token): @bp.route("/workspaces//invitations//resend", methods=["POST"]) def resend_invitation(workspace_id, token): invite = Invitations.resend(g.current_user, workspace_id, token) - send_invite_email(g.current_user.full_name, invite.token, invite.user_email) + send_invite_email(g.current_user.full_name, invite.token, invite.email) return redirect( url_for( "workspaces.workspace_members", diff --git a/tests/domain/test_invitations.py b/tests/domain/test_invitations.py index e63258e8..4f4a1492 100644 --- a/tests/domain/test_invitations.py +++ b/tests/domain/test_invitations.py @@ -22,7 +22,7 @@ def test_create_invitation(): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) - invite = Invitations.create(workspace.owner, ws_role) + invite = Invitations.create(workspace.owner, ws_role, user.email) assert invite.user == user assert invite.workspace_role == ws_role assert invite.inviter == workspace.owner @@ -34,7 +34,7 @@ def test_accept_invitation(): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) - invite = Invitations.create(workspace.owner, ws_role) + invite = Invitations.create(workspace.owner, ws_role, user.email) assert invite.is_pending accepted_invite = Invitations.accept(user, invite.token) assert accepted_invite.is_accepted @@ -89,7 +89,7 @@ def test_accept_invitation_twice(): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) - invite = Invitations.create(workspace.owner, ws_role) + invite = Invitations.create(workspace.owner, ws_role, user.email) Invitations.accept(user, invite.token) with pytest.raises(InvitationError): Invitations.accept(user, invite.token) @@ -99,7 +99,7 @@ def test_revoke_invitation(): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) - invite = Invitations.create(workspace.owner, ws_role) + invite = Invitations.create(workspace.owner, ws_role, user.email) assert invite.is_pending Invitations.revoke(invite.token) assert invite.is_revoked @@ -109,7 +109,7 @@ def test_resend_invitation(): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) - invite = Invitations.create(workspace.owner, ws_role) + invite = Invitations.create(workspace.owner, ws_role, user.email) Invitations.resend(workspace.owner, workspace.id, invite.token) assert ws_role.invitations[0].is_revoked assert ws_role.invitations[1].is_pending diff --git a/tests/factories.py b/tests/factories.py index 7c7bdfd8..96e9c22c 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -342,5 +342,6 @@ class InvitationFactory(Base): class Meta: model = Invitation + email = factory.Faker("email") status = InvitationStatus.PENDING expiration_time = Invitations.current_expiration_time() diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index 1a91e723..85d30318 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -502,3 +502,31 @@ def test_resend_invitation_sends_email(client, user_session, queue): ) assert len(queue.get_queue()) == 1 + + +def test_existing_member_invite_resent_to_email_submitted_in_form( + client, user_session, queue +): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + ws_role = WorkspaceRoleFactory.create( + user=user, workspace=workspace, status=WorkspaceRoleStatus.PENDING + ) + invite = InvitationFactory.create( + user_id=user.id, + workspace_role_id=ws_role.id, + status=InvitationStatus.PENDING, + email="example@example.com", + ) + user_session(workspace.owner) + client.post( + url_for( + "workspaces.resend_invitation", + workspace_id=workspace.id, + token=invite.token, + ) + ) + + assert user.email != "example@example.com" + assert len(queue.get_queue().jobs[0].args[0]) == 1 + assert queue.get_queue().jobs[0].args[0][0] == "example@example.com" From 8ea5e6a805599099af5d1f606cee925bddc83c7a Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Wed, 21 Nov 2018 15:13:53 -0500 Subject: [PATCH 16/17] Update test to make it clearer --- tests/routes/test_workspaces.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index 85d30318..c15ee2c5 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -527,6 +527,7 @@ def test_existing_member_invite_resent_to_email_submitted_in_form( ) ) + send_mail_job = queue.get_queue().jobs[0] assert user.email != "example@example.com" - assert len(queue.get_queue().jobs[0].args[0]) == 1 - assert queue.get_queue().jobs[0].args[0][0] == "example@example.com" + assert send_mail_job.func.__func__.__name__ == "_send_mail" + assert send_mail_job.args[0] == ["example@example.com"] From d0c6ea5dbdb3e8c5988e3e3176438ec82834a81f Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Tue, 27 Nov 2018 17:03:31 -0500 Subject: [PATCH 17/17] Update migrations after rebase --- alembic/versions/02d11579a581_add_email_to_invite.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/02d11579a581_add_email_to_invite.py b/alembic/versions/02d11579a581_add_email_to_invite.py index 3788c112..cd21df51 100644 --- a/alembic/versions/02d11579a581_add_email_to_invite.py +++ b/alembic/versions/02d11579a581_add_email_to_invite.py @@ -1,7 +1,7 @@ """Add email to invite Revision ID: 02d11579a581 -Revises: 4c0b8263d800 +Revises: 4f46aecb337f Create Date: 2018-11-19 14:51:33.178358 """ @@ -12,7 +12,7 @@ from sqlalchemy.sql import text # revision identifiers, used by Alembic. revision = '02d11579a581' -down_revision = '4c0b8263d800' +down_revision = '4f46aecb337f' branch_labels = None depends_on = None