From 755fabd7250d9a1630adc117e7ade007446569b5 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 31 May 2019 12:34:37 -0400 Subject: [PATCH 1/5] Database changes for new portfolio invitation flow. - user_id is nullable on role tables - invitation tables hold basic user information --- ...ae6_add_user_data_fields_to_invitations.py | 54 +++++++++++++++++++ atst/models/application_role.py | 2 +- atst/models/mixins/invites.py | 5 ++ atst/models/portfolio_role.py | 2 +- 4 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py diff --git a/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py b/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py new file mode 100644 index 00000000..1ba387fd --- /dev/null +++ b/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py @@ -0,0 +1,54 @@ +"""add user data fields to invitations + +Revision ID: 8467440c4ae6 +Revises: d2390c547dca +Create Date: 2019-05-31 12:40:10.457529 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = '8467440c4ae6' +down_revision = 'd2390c547dca' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('application_invitations', sa.Column('dod_id', sa.String(), nullable=True)) + op.add_column('application_invitations', sa.Column('first_name', sa.String(), nullable=True)) + op.add_column('application_invitations', sa.Column('last_name', sa.String(), nullable=True)) + op.add_column('application_invitations', sa.Column('phone_number', sa.String(), nullable=True)) + op.alter_column('application_roles', 'user_id', + existing_type=postgresql.UUID(), + nullable=True) + op.add_column('portfolio_invitations', sa.Column('dod_id', sa.String(), nullable=True)) + op.add_column('portfolio_invitations', sa.Column('first_name', sa.String(), nullable=True)) + op.add_column('portfolio_invitations', sa.Column('last_name', sa.String(), nullable=True)) + op.add_column('portfolio_invitations', sa.Column('phone_number', sa.String(), nullable=True)) + op.alter_column('portfolio_roles', 'user_id', + existing_type=postgresql.UUID(), + nullable=True) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('portfolio_roles', 'user_id', + existing_type=postgresql.UUID(), + nullable=True) + op.drop_column('portfolio_invitations', 'phone_number') + op.drop_column('portfolio_invitations', 'last_name') + op.drop_column('portfolio_invitations', 'first_name') + op.drop_column('portfolio_invitations', 'dod_id') + op.alter_column('application_roles', 'user_id', + existing_type=postgresql.UUID(), + nullable=True) + op.drop_column('application_invitations', 'phone_number') + op.drop_column('application_invitations', 'last_name') + op.drop_column('application_invitations', 'first_name') + op.drop_column('application_invitations', 'dod_id') + # ### end Alembic commands ### diff --git a/atst/models/application_role.py b/atst/models/application_role.py index 7b343cd8..9bd99699 100644 --- a/atst/models/application_role.py +++ b/atst/models/application_role.py @@ -42,7 +42,7 @@ class ApplicationRole( application = relationship("Application", back_populates="roles") user_id = Column( - UUID(as_uuid=True), ForeignKey("users.id"), index=True, nullable=False + UUID(as_uuid=True), ForeignKey("users.id"), index=True, nullable=True ) status = Column(SQLAEnum(Status, native_enum=False), default=Status.PENDING) diff --git a/atst/models/mixins/invites.py b/atst/models/mixins/invites.py index f7212850..3ea84aed 100644 --- a/atst/models/mixins/invites.py +++ b/atst/models/mixins/invites.py @@ -45,6 +45,11 @@ class InvitesMixin(object): email = Column(String, nullable=False) + dod_id = Column(String) + first_name = Column(String) + last_name = Column(String) + phone_number = Column(String) + def __repr__(self): role_id = self.role.id if self.role else None return "<{}(user='{}', role='{}', id='{}', email='{}')>".format( diff --git a/atst/models/portfolio_role.py b/atst/models/portfolio_role.py index 639680eb..c5e1fca8 100644 --- a/atst/models/portfolio_role.py +++ b/atst/models/portfolio_role.py @@ -48,7 +48,7 @@ class PortfolioRole( portfolio = relationship("Portfolio", back_populates="roles") user_id = Column( - UUID(as_uuid=True), ForeignKey("users.id"), index=True, nullable=False + UUID(as_uuid=True), ForeignKey("users.id"), index=True, nullable=True ) status = Column(SQLAEnum(Status, native_enum=False), default=Status.PENDING) From c085db23d7ebeee8f02d16b8fc6089607b50c242 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 31 May 2019 12:58:23 -0400 Subject: [PATCH 2/5] New invitation backend for portfolio invitations. Portfolio invitations do not associate a user entity until the invitation has been accepted. User info, including DOD ID, is held on the invitation itself. When a user accepts and invitation, their user entry is associated with the corresponding `portfolio_role` entry. The same change will be applied to `application_role` and application invitations. For now, small changes have been made to application-related methods so that that flow works as-is. --- ...ae6_add_user_data_fields_to_invitations.py | 4 +- atst/domain/application_roles.py | 3 +- atst/domain/invitations.py | 40 ++++++---- atst/domain/portfolio_roles.py | 3 +- atst/domain/portfolios/portfolios.py | 34 ++++----- atst/models/mixins/invites.py | 2 +- atst/models/portfolio_role.py | 13 ++-- atst/routes/portfolios/members.py | 24 ++++-- atst/services/invitation.py | 8 +- atst/utils/flash.py | 2 +- tests/domain/test_application_roles.py | 2 +- tests/domain/test_invitations.py | 76 ++++++++----------- tests/domain/test_portfolios.py | 43 ++++------- tests/models/test_portfolio_role.py | 5 +- tests/routes/applications/test_invitations.py | 4 +- tests/routes/portfolios/test_invitations.py | 32 ++------ tests/routes/portfolios/test_members.py | 33 ++++---- 17 files changed, 163 insertions(+), 165 deletions(-) diff --git a/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py b/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py index 1ba387fd..50f79f6a 100644 --- a/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py +++ b/alembic/versions/8467440c4ae6_add_user_data_fields_to_invitations.py @@ -1,7 +1,7 @@ """add user data fields to invitations Revision ID: 8467440c4ae6 -Revises: d2390c547dca +Revises: 24700d113ea9 Create Date: 2019-05-31 12:40:10.457529 """ @@ -11,7 +11,7 @@ from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. revision = '8467440c4ae6' -down_revision = 'd2390c547dca' +down_revision = '24700d113ea9' branch_labels = None depends_on = None diff --git a/atst/domain/application_roles.py b/atst/domain/application_roles.py index 177d49f0..2478d351 100644 --- a/atst/domain/application_roles.py +++ b/atst/domain/application_roles.py @@ -28,8 +28,9 @@ class ApplicationRoles(object): return application_role @classmethod - def enable(cls, role): + def enable(cls, role, user): role.status = ApplicationRoleStatus.ACTIVE + role.user = user db.session.add(role) db.session.commit() diff --git a/atst/domain/invitations.py b/atst/domain/invitations.py index c38e88d8..6b68db92 100644 --- a/atst/domain/invitations.py +++ b/atst/domain/invitations.py @@ -55,7 +55,7 @@ class BaseInvitations(object): return invite @classmethod - def create(cls, inviter, role, email): + def create(cls, inviter, role, member_data, commit=False): # pylint: disable=not-callable invite = cls.model( role=role, @@ -63,10 +63,16 @@ class BaseInvitations(object): user=role.user, status=InvitationStatus.PENDING, expiration_time=cls.current_expiration_time(), - email=email, + email=member_data.get("email"), + dod_id=member_data.get("dod_id"), + first_name=member_data.get("first_name"), + phone_number=member_data.get("phone_number"), + last_name=member_data.get("last_name"), ) db.session.add(invite) - db.session.commit() + + if commit: + db.session.commit() return invite @@ -74,7 +80,7 @@ class BaseInvitations(object): def accept(cls, user, token): invite = cls._get(token) - if invite.user.dod_id != user.dod_id: + if invite.dod_id != user.dod_id: if invite.is_pending: cls._update_status(invite, InvitationStatus.REJECTED_WRONG_USER) raise WrongUserError(user, invite) @@ -88,7 +94,7 @@ class BaseInvitations(object): elif invite.is_pending: # pragma: no branch cls._update_status(invite, InvitationStatus.ACCEPTED) - cls.role_domain_class.enable(invite.role) + cls.role_domain_class.enable(invite.role, user) return invite @classmethod @@ -111,20 +117,22 @@ class BaseInvitations(object): return cls._update_status(invite, InvitationStatus.REVOKED) @classmethod - def lookup_by_resource_and_user(cls, resource, user): - role = cls.role_domain_class.get(resource.id, user.id) - - if role.latest_invitation is None: - raise NotFoundError(cls.model.__tablename__) - - return role.latest_invitation - - @classmethod - def resend(cls, user, token): + def resend(cls, inviter, token): previous_invitation = cls._get(token) cls._update_status(previous_invitation, InvitationStatus.REVOKED) - return cls.create(user, previous_invitation.role, previous_invitation.email) + return cls.create( + inviter, + previous_invitation.role, + { + "email": previous_invitation.email, + "dod_id": previous_invitation.dod_id, + "first_name": previous_invitation.first_name, + "last_name": previous_invitation.last_name, + "phone_number": previous_invitation.last_name, + }, + commit=True, + ) class PortfolioInvitations(BaseInvitations): diff --git a/atst/domain/portfolio_roles.py b/atst/domain/portfolio_roles.py index eee9fe3f..d20625c2 100644 --- a/atst/domain/portfolio_roles.py +++ b/atst/domain/portfolio_roles.py @@ -163,8 +163,9 @@ class PortfolioRoles(object): return portfolio_role @classmethod - def enable(cls, portfolio_role): + def enable(cls, portfolio_role, user): portfolio_role.status = PortfolioRoleStatus.ACTIVE + portfolio_role.user = user db.session.add(portfolio_role) db.session.commit() diff --git a/atst/domain/portfolios/portfolios.py b/atst/domain/portfolios/portfolios.py index 49e5b005..db78f6f1 100644 --- a/atst/domain/portfolios/portfolios.py +++ b/atst/domain/portfolios/portfolios.py @@ -1,10 +1,9 @@ from atst.domain.permission_sets import PermissionSets from atst.domain.authz import Authorization -from atst.models.permissions import Permissions -from atst.domain.users import Users from atst.domain.portfolio_roles import PortfolioRoles +from atst.domain.invitations import PortfolioInvitations from atst.domain.environments import Environments -from atst.models.portfolio_role import Status as PortfolioRoleStatus +from atst.models import Permissions, PortfolioRole, PortfolioRoleStatus from .query import PortfoliosQuery from .scopes import ScopedPortfolio @@ -49,25 +48,26 @@ class Portfolios(object): portfolios = PortfoliosQuery.get_for_user(user) return portfolios - @classmethod - def create_member(cls, portfolio, data): - new_user = Users.get_or_create_by_dod_id( - data["dod_id"], - first_name=data["first_name"], - last_name=data["last_name"], - email=data["email"], - provisional=True, - ) - permission_sets = data.get("permission_sets", []) - return Portfolios.add_member( - portfolio, new_user, permission_sets=permission_sets - ) - @classmethod def add_member(cls, portfolio, member, permission_sets=None): portfolio_role = PortfolioRoles.add(member, portfolio.id, permission_sets) return portfolio_role + @classmethod + def invite(cls, portfolio, inviter, member_data): + permission_sets = PortfolioRoles._permission_sets_for_names( + member_data.get("permission_sets", []) + ) + role = PortfolioRole(portfolio_id=portfolio.id, permission_sets=permission_sets) + + invitation = PortfolioInvitations.create( + inviter=inviter, role=role, member_data=member_data + ) + + PortfoliosQuery.add_and_commit(role) + + return invitation + @classmethod def update_member(cls, member, permission_sets): return PortfolioRoles.update(member, permission_sets) diff --git a/atst/models/mixins/invites.py b/atst/models/mixins/invites.py index 3ea84aed..f6c17f43 100644 --- a/atst/models/mixins/invites.py +++ b/atst/models/mixins/invites.py @@ -97,7 +97,7 @@ class InvitesMixin(object): @property def user_name(self): - return self.role.user.full_name + return "{} {}".format(self.first_name, self.last_name) @property def is_revokable(self): diff --git a/atst/models/portfolio_role.py b/atst/models/portfolio_role.py index c5e1fca8..c88fd88b 100644 --- a/atst/models/portfolio_role.py +++ b/atst/models/portfolio_role.py @@ -116,7 +116,14 @@ class PortfolioRole( @property def user_name(self): - return self.user.full_name + if self.user: + return self.user.full_name + else: + return self.latest_invitation.user_name + + @property + def full_name(self): + return self.user_name @property def is_active(self): @@ -128,10 +135,6 @@ class PortfolioRole( self.latest_invitation and self.latest_invitation.is_inactive ) - @property - def full_name(self): - return self.user.full_name - @property def application_id(self): return None diff --git a/atst/routes/portfolios/members.py b/atst/routes/portfolios/members.py index a823eac1..2ba90d86 100644 --- a/atst/routes/portfolios/members.py +++ b/atst/routes/portfolios/members.py @@ -3,12 +3,23 @@ from flask import render_template, request as http_request, g, redirect, url_for from . import portfolios_bp from atst.domain.exceptions import AlreadyExistsError from atst.domain.portfolios import Portfolios -from atst.services.invitation import Invitation as InvitationService import atst.forms.portfolio_member as member_forms from atst.domain.authz.decorator import user_can_access_decorator as user_can from atst.models.permissions import Permissions from atst.utils.flash import formatted_flash as flash +from atst.queue import queue + + +def send_portfolio_invitation(invitee_email, inviter_name, token): + body = render_template( + "emails/portfolio/invitation.txt", owner=inviter_name, token=token + ) + queue.send_mail( + [invitee_email], + "{} has invited you to a JEDI cloud portfolio".format(inviter_name), + body, + ) @portfolios_bp.route("/portfolios//members/new", methods=["POST"]) @@ -19,13 +30,14 @@ def create_member(portfolio_id): if form.validate(): try: - member = Portfolios.create_member(portfolio, form.update_data) - invite_service = InvitationService( - g.current_user, member, form.update_data.get("email") + invite = Portfolios.invite(portfolio, g.current_user, form.update_data) + send_portfolio_invitation( + invite.email, g.current_user.full_name, invite.token ) - invite_service.invite() - flash("new_portfolio_member", new_member=member, portfolio=portfolio) + flash( + "new_portfolio_member", user_name=invite.user_name, portfolio=portfolio + ) except AlreadyExistsError: return render_template( diff --git a/atst/services/invitation.py b/atst/services/invitation.py index e9dec252..17d3b922 100644 --- a/atst/services/invitation.py +++ b/atst/services/invitation.py @@ -79,7 +79,13 @@ class Invitation: return invite def _create_invite(self): - return self.domain_class.create(self.inviter, self.member, self.email) + user = self.member.user + return self.domain_class.create( + self.inviter, + self.member, + {"email": self.email, "dod_id": user.dod_id}, + commit=True, + ) def _send_invite_email(self, token): body = render_template( diff --git a/atst/utils/flash.py b/atst/utils/flash.py index dc63dcd2..b16dee89 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -61,7 +61,7 @@ MESSAGES = { "new_portfolio_member": { "title_template": translate("flash.success"), "message_template": """ -

{{ "flash.new_portfolio_member" | translate({ "user_name": new_member.user_name }) }}

+

{{ "flash.new_portfolio_member" | translate({ "user_name": user_name }) }}

""", "category": "success", }, diff --git a/tests/domain/test_application_roles.py b/tests/domain/test_application_roles.py index d6f2db69..3129d91e 100644 --- a/tests/domain/test_application_roles.py +++ b/tests/domain/test_application_roles.py @@ -33,7 +33,7 @@ def test_enabled_application_role(): ) assert app_role.status == ApplicationRoleStatus.DISABLED - ApplicationRoles.enable(app_role) + ApplicationRoles.enable(app_role, app_role.user) assert app_role.status == ApplicationRoleStatus.ACTIVE diff --git a/tests/domain/test_invitations.py b/tests/domain/test_invitations.py index 902afe2b..358ab7d6 100644 --- a/tests/domain/test_invitations.py +++ b/tests/domain/test_invitations.py @@ -23,10 +23,11 @@ from tests.factories import ( def test_create_invitation(): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) - assert invite.user == user - assert invite.role == ws_role + role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + invite = PortfolioInvitations.create( + portfolio.owner, role, user.to_dictionary(), commit=True + ) + assert invite.role == role assert invite.inviter == portfolio.owner assert invite.status == InvitationStatus.PENDING assert re.match(r"^[\w\-_]+$", invite.token) @@ -35,8 +36,10 @@ def test_create_invitation(): def test_accept_invitation(): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) + role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + invite = PortfolioInvitations.create( + portfolio.owner, role, user.to_dictionary(), commit=True + ) assert invite.is_pending accepted_invite = PortfolioInvitations.accept(user, invite.token) assert accepted_invite.is_accepted @@ -45,14 +48,14 @@ def test_accept_invitation(): def test_accept_expired_invitation(): user = UserFactory.create() portfolio = PortfolioFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + role = PortfolioRoleFactory.create(portfolio=portfolio) increment = PortfolioInvitations.EXPIRATION_LIMIT_MINUTES + 1 expiration_time = datetime.datetime.now() - datetime.timedelta(minutes=increment) invite = PortfolioInvitationFactory.create( - user=user, expiration_time=expiration_time, status=InvitationStatus.PENDING, - role=ws_role, + role=role, + dod_id=user.dod_id, ) with pytest.raises(ExpiredError): PortfolioInvitations.accept(user, invite.token) @@ -63,9 +66,9 @@ def test_accept_expired_invitation(): def test_accept_rejected_invite(): user = UserFactory.create() portfolio = PortfolioFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + role = PortfolioRoleFactory.create(portfolio=portfolio) invite = PortfolioInvitationFactory.create( - user=user, status=InvitationStatus.REJECTED_EXPIRED, role=ws_role + status=InvitationStatus.REJECTED_EXPIRED, role=role, dod_id=user.dod_id ) with pytest.raises(InvitationError): PortfolioInvitations.accept(user, invite.token) @@ -74,9 +77,9 @@ def test_accept_rejected_invite(): def test_accept_revoked_invite(): user = UserFactory.create() portfolio = PortfolioFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + role = PortfolioRoleFactory.create(portfolio=portfolio) invite = PortfolioInvitationFactory.create( - user=user, status=InvitationStatus.REVOKED, role=ws_role + status=InvitationStatus.REVOKED, role=role, dod_id=user.dod_id ) with pytest.raises(InvitationError): PortfolioInvitations.accept(user, invite.token) @@ -85,9 +88,9 @@ def test_accept_revoked_invite(): def test_wrong_user_accepts_invitation(): user = UserFactory.create() portfolio = PortfolioFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + role = PortfolioRoleFactory.create(portfolio=portfolio) wrong_user = UserFactory.create() - invite = PortfolioInvitationFactory.create(user=user, role=ws_role) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) with pytest.raises(WrongUserError): PortfolioInvitations.accept(wrong_user, invite.token) @@ -95,9 +98,9 @@ def test_wrong_user_accepts_invitation(): def test_user_cannot_accept_invitation_accepted_by_wrong_user(): user = UserFactory.create() portfolio = PortfolioFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + role = PortfolioRoleFactory.create(portfolio=portfolio) wrong_user = UserFactory.create() - invite = PortfolioInvitationFactory.create(user=user, role=ws_role) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) with pytest.raises(WrongUserError): PortfolioInvitations.accept(wrong_user, invite.token) with pytest.raises(InvitationError): @@ -107,8 +110,8 @@ def test_user_cannot_accept_invitation_accepted_by_wrong_user(): def test_accept_invitation_twice(): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) + role = PortfolioRoleFactory.create(portfolio=portfolio) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) PortfolioInvitations.accept(user, invite.token) with pytest.raises(InvitationError): PortfolioInvitations.accept(user, invite.token) @@ -117,44 +120,31 @@ def test_accept_invitation_twice(): def test_revoke_invitation(): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) + role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) assert invite.is_pending PortfolioInvitations.revoke(invite.token) assert invite.is_revoked -def test_resend_invitation(): +def test_resend_invitation(session): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) - PortfolioInvitations.resend(user, invite.token) - assert ws_role.invitations[0].is_revoked - assert ws_role.invitations[1].is_pending + role = PortfolioRoleFactory.create(portfolio=portfolio) + first_invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) + assert first_invite.is_pending + second_invite = PortfolioInvitations.resend(user, first_invite.token) + assert first_invite.is_revoked + assert second_invite.is_pending def test_audit_event_for_accepted_invite(): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) + role = PortfolioRoleFactory.create(portfolio=portfolio) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user.dod_id) invite = PortfolioInvitations.accept(user, invite.token) accepted_event = AuditLog.get_by_resource(invite.id)[0] assert "email" in accepted_event.event_details assert "dod_id" in accepted_event.event_details - - -def test_lookup_by_user_and_portfolio(): - portfolio = PortfolioFactory.create() - user = UserFactory.create() - ws_role = PortfolioRoleFactory.create(user=user, portfolio=portfolio) - invite = PortfolioInvitations.create(portfolio.owner, ws_role, user.email) - - assert PortfolioInvitations.lookup_by_resource_and_user(portfolio, user) == invite - - with pytest.raises(NotFoundError): - PortfolioInvitations.lookup_by_resource_and_user( - portfolio, UserFactory.create() - ) diff --git a/tests/domain/test_portfolios.py b/tests/domain/test_portfolios.py index 1d28b013..64454c86 100644 --- a/tests/domain/test_portfolios.py +++ b/tests/domain/test_portfolios.py @@ -49,36 +49,6 @@ def test_portfolio_has_timestamps(portfolio): assert portfolio.time_created == portfolio.time_updated -def test_can_create_portfolio_role(portfolio, portfolio_owner): - user_data = { - "first_name": "New", - "last_name": "User", - "email": "new.user@mail.com", - "portfolio_role": "developer", - "dod_id": "1234567890", - } - - new_member = Portfolios.create_member(portfolio, user_data) - assert new_member.portfolio == portfolio - assert new_member.user.provisional - - -def test_can_add_existing_user_to_portfolio(portfolio, portfolio_owner): - user = UserFactory.create() - user_data = { - "first_name": "New", - "last_name": "User", - "email": "new.user@mail.com", - "portfolio_role": "developer", - "dod_id": user.dod_id, - } - - new_member = Portfolios.create_member(portfolio, user_data) - assert new_member.portfolio == portfolio - assert new_member.user.email == user.email - assert not new_member.user.provisional - - def test_update_portfolio_role_role(portfolio, portfolio_owner): user_data = { "first_name": "New", @@ -238,3 +208,16 @@ def test_does_not_count_disabled_members(session): ) assert portfolio.user_count == 3 + + +def test_invite(): + portfolio = PortfolioFactory.create() + inviter = UserFactory.create() + member_data = UserFactory.dictionary() + + invitation = Portfolios.invite(portfolio, inviter, member_data) + + assert invitation.role + assert invitation.role.portfolio == portfolio + assert invitation.role.user is None + assert invitation.dod_id == member_data["dod_id"] diff --git a/tests/models/test_portfolio_role.py b/tests/models/test_portfolio_role.py index 1f586db3..f5fb2bf4 100644 --- a/tests/models/test_portfolio_role.py +++ b/tests/models/test_portfolio_role.py @@ -65,7 +65,10 @@ def test_has_portfolio_status_history(session): # to commit after create() PortfolioRoleFactory._meta.sqlalchemy_session_persistence = "flush" portfolio_role = PortfolioRoleFactory.create(portfolio=portfolio, user=user) - PortfolioRoles.enable(portfolio_role) + portfolio_role.status = PortfolioRoleStatus.ACTIVE + session.add(portfolio_role) + session.commit() + changed_events = ( session.query(AuditEvent) .filter( diff --git a/tests/routes/applications/test_invitations.py b/tests/routes/applications/test_invitations.py index 9ca6d23b..85674e6d 100644 --- a/tests/routes/applications/test_invitations.py +++ b/tests/routes/applications/test_invitations.py @@ -8,7 +8,7 @@ def test_accept_application_invitation(client, user_session): application = ApplicationFactory.create() app_role = ApplicationRoleFactory.create(application=application, user=user) invite = ApplicationInvitationFactory.create( - role=app_role, user=user, inviter=application.portfolio.owner + role=app_role, inviter=application.portfolio.owner, dod_id=user.dod_id ) user_session(user) @@ -28,7 +28,7 @@ def test_accept_application_invitation_end_to_end(client, user_session): application = ApplicationFactory.create(name="Millenium Falcon") app_role = ApplicationRoleFactory.create(application=application, user=user) invite = ApplicationInvitationFactory.create( - role=app_role, user=user, inviter=application.portfolio.owner + role=app_role, dod_id=user.dod_id, inviter=application.portfolio.owner ) user_session(user) diff --git a/tests/routes/portfolios/test_invitations.py b/tests/routes/portfolios/test_invitations.py index 817e8a96..cbc5594f 100644 --- a/tests/routes/portfolios/test_invitations.py +++ b/tests/routes/portfolios/test_invitations.py @@ -18,10 +18,10 @@ from atst.domain.permission_sets import PermissionSets def test_existing_member_accepts_valid_invite(client, user_session): portfolio = PortfolioFactory.create() user = UserFactory.create() - ws_role = PortfolioRoleFactory.create( + role = PortfolioRoleFactory.create( portfolio=portfolio, user=user, status=PortfolioRoleStatus.PENDING ) - invite = PortfolioInvitationFactory.create(user_id=user.id, role=ws_role) + invite = PortfolioInvitationFactory.create(dod_id=user.dod_id, role=role) # the user does not have access to the portfolio before accepting the invite assert len(Portfolios.for_user(user)) == 0 @@ -46,32 +46,15 @@ def test_existing_member_accepts_valid_invite(client, user_session): def test_new_member_accepts_valid_invite(monkeypatch, client, user_session): portfolio = PortfolioFactory.create() user_info = UserFactory.dictionary() - - user_session(portfolio.owner) - response = client.post( - url_for("portfolios.create_member", portfolio_id=portfolio.id), - data={ - "permission_sets-perms_app_mgmt": PermissionSets.VIEW_PORTFOLIO_APPLICATION_MANAGEMENT, - "permission_sets-perms_funding": PermissionSets.VIEW_PORTFOLIO_FUNDING, - "permission_sets-perms_reporting": PermissionSets.VIEW_PORTFOLIO_REPORTS, - "permission_sets-perms_portfolio_mgmt": PermissionSets.VIEW_PORTFOLIO_ADMIN, - "user_data-first_name": user_info["first_name"], - "user_data-last_name": user_info["last_name"], - "user_data-dod_id": user_info["dod_id"], - "user_data-email": user_info["email"], - }, - ) - - assert response.status_code == 302 - user = Users.get_by_dod_id(user_info["dod_id"]) - token = user.portfolio_invitations[0].token + role = PortfolioRoleFactory.create(portfolio=portfolio) + invite = PortfolioInvitationFactory.create(role=role, dod_id=user_info["dod_id"]) monkeypatch.setattr( "atst.domain.auth.should_redirect_to_user_profile", lambda *args: False ) - user_session(user) + user_session(UserFactory.create(dod_id=user_info["dod_id"])) response = client.get( - url_for("portfolios.accept_invitation", portfolio_token=token) + url_for("portfolios.accept_invitation", portfolio_token=invite.token) ) # user is redirected to the portfolio view @@ -81,7 +64,8 @@ def test_new_member_accepts_valid_invite(monkeypatch, client, user_session): in response.headers["Location"] ) # the user has access to the portfolio - assert len(Portfolios.for_user(user)) == 1 + assert role.user.dod_id == user_info["dod_id"] + assert len(role.user.portfolio_roles) == 1 def test_member_accepts_invalid_invite(client, user_session): diff --git a/tests/routes/portfolios/test_members.py b/tests/routes/portfolios/test_members.py index d2c4ec67..cd046b6a 100644 --- a/tests/routes/portfolios/test_members.py +++ b/tests/routes/portfolios/test_members.py @@ -1,9 +1,11 @@ from flask import url_for -from tests.factories import UserFactory, PortfolioFactory from atst.domain.permission_sets import PermissionSets +from atst.models import PortfolioInvitation from atst.queue import queue +from tests.factories import UserFactory, PortfolioFactory + _DEFAULT_PERMS_FORM_DATA = { "permission_sets-perms_app_mgmt": PermissionSets.VIEW_PORTFOLIO_APPLICATION_MANAGEMENT, "permission_sets-perms_funding": PermissionSets.VIEW_PORTFOLIO_FUNDING, @@ -23,8 +25,8 @@ def test_user_with_permission_has_add_member_link(client, user_session): ) -def test_create_member(client, user_session): - user = UserFactory.create() +def test_create_member(client, user_session, session): + user_data = UserFactory.dictionary() portfolio = PortfolioFactory.create() user_session(portfolio.owner) queue_length = len(queue.get_queue()) @@ -32,20 +34,25 @@ def test_create_member(client, user_session): response = client.post( url_for("portfolios.create_member", portfolio_id=portfolio.id), data={ - "user_data-dod_id": user.dod_id, - "user_data-first_name": "user_data-Wilbur", - "user_data-last_name": "user_data-Zuckerman", - "user_data-email": "user_data-some_pig@zuckermans.com", - "user_data-portfolio_role": "user_data-developer", + "user_data-dod_id": user_data.get("dod_id"), + "user_data-first_name": user_data.get("first_name"), + "user_data-last_name": user_data.get("last_name"), + "user_data-email": user_data.get("email"), **_DEFAULT_PERMS_FORM_DATA, }, follow_redirects=True, ) assert response.status_code == 200 - assert user.full_name in response.data.decode() - assert user.has_portfolios - assert user.portfolio_invitations + full_name = "{} {}".format(user_data.get("first_name"), user_data.get("last_name")) + assert full_name in response.data.decode() + + invitation = ( + session.query(PortfolioInvitation) + .filter_by(dod_id=user_data.get("dod_id")) + .one() + ) + assert invitation.role.portfolio == portfolio + assert len(queue.get_queue()) == queue_length + 1 - portfolio_role = user.portfolio_roles[0] - assert len(portfolio_role.permission_sets) == 5 + assert len(invitation.role.permission_sets) == 5 From 25ad9ebcd6525d99d858613a6b314535d81d637a Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 3 Jun 2019 09:13:57 -0400 Subject: [PATCH 3/5] Do not check if user is a task order officer when user accepts an invite. That functionality is deprecated for now. --- atst/routes/portfolios/invitations.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/atst/routes/portfolios/invitations.py b/atst/routes/portfolios/invitations.py index dd1f1c5a..c95fc8a8 100644 --- a/atst/routes/portfolios/invitations.py +++ b/atst/routes/portfolios/invitations.py @@ -23,12 +23,6 @@ def send_invite_email(owner_name, token, new_member_email): def accept_invitation(portfolio_token): invite = PortfolioInvitations.accept(g.current_user, portfolio_token) - for task_order in invite.portfolio.task_orders: - if g.current_user in task_order.officers: - return redirect( - url_for("task_orders.view_task_order", task_order_id=task_order.id) - ) - return redirect( url_for("applications.portfolio_applications", portfolio_id=invite.portfolio.id) ) From 5434443b02107c636a5e1769a9fb2cbf71b74e90 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 3 Jun 2019 09:39:14 -0400 Subject: [PATCH 4/5] Adjust `script/seed_sample.py` for new portfolio invitation flow. --- script/seed_sample.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/script/seed_sample.py b/script/seed_sample.py index a4284ead..a0becb96 100644 --- a/script/seed_sample.py +++ b/script/seed_sample.py @@ -152,9 +152,9 @@ def get_users(): def add_members_to_portfolio(portfolio): for user_data in PORTFOLIO_USERS: - ws_role = Portfolios.create_member(portfolio, user_data) - db.session.refresh(ws_role) - PortfolioRoles.enable(ws_role) + invite = Portfolios.invite(portfolio, portfolio.owner, user_data) + user = Users.get_or_create_by_dod_id(user_data["dod_id"]) + PortfolioRoles.enable(invite.role, user) db.session.commit() From 4f8e9cddc83eaa3a2edb4d7e2382f479a6473ec4 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 3 Jun 2019 12:55:15 -0400 Subject: [PATCH 5/5] Merge portfolio members routes with invitations. `portfolios.create_member` now just sends an invitation, so it should be with the invitation routes. This also de-duplicates the function for sending a portfolio invitation email. --- atst/routes/portfolios/__init__.py | 1 - atst/routes/portfolios/invitations.py | 54 ++++++++++++--- atst/routes/portfolios/members.py | 57 ---------------- .../admin/add_new_portfolio_member.html | 2 +- tests/routes/portfolios/test_invitations.py | 66 ++++++++++++++++--- tests/routes/portfolios/test_members.py | 58 ---------------- tests/test_access.py | 6 +- 7 files changed, 106 insertions(+), 138 deletions(-) delete mode 100644 atst/routes/portfolios/members.py delete mode 100644 tests/routes/portfolios/test_members.py diff --git a/atst/routes/portfolios/__init__.py b/atst/routes/portfolios/__init__.py index 979f9ea7..bceb9100 100644 --- a/atst/routes/portfolios/__init__.py +++ b/atst/routes/portfolios/__init__.py @@ -4,7 +4,6 @@ from operator import attrgetter portfolios_bp = Blueprint("portfolios", __name__) from . import index -from . import members from . import invitations from . import admin from atst.utils.context_processors import portfolio as portfolio_context_processor diff --git a/atst/routes/portfolios/invitations.py b/atst/routes/portfolios/invitations.py index c95fc8a8..00af9377 100644 --- a/atst/routes/portfolios/invitations.py +++ b/atst/routes/portfolios/invitations.py @@ -1,20 +1,23 @@ -from flask import g, redirect, url_for, render_template +from flask import g, redirect, url_for, render_template, request as http_request from . import portfolios_bp +from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.domain.exceptions import AlreadyExistsError from atst.domain.invitations import PortfolioInvitations +from atst.domain.portfolios import Portfolios +from atst.models import Permissions from atst.queue import queue from atst.utils.flash import formatted_flash as flash -from atst.domain.authz.decorator import user_can_access_decorator as user_can -from atst.models.permissions import Permissions +import atst.forms.portfolio_member as member_forms -def send_invite_email(owner_name, token, new_member_email): +def send_portfolio_invitation(invitee_email, inviter_name, token): body = render_template( - "emails/portfolio/invitation.txt", owner=owner_name, token=token + "emails/portfolio/invitation.txt", owner=inviter_name, token=token ) queue.send_mail( - [new_member_email], - "{} has invited you to a JEDI Cloud Portfolio".format(owner_name), + [invitee_email], + "{} has invited you to a JEDI cloud portfolio".format(inviter_name), body, ) @@ -51,7 +54,7 @@ def revoke_invitation(portfolio_id, portfolio_token): @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="resend invitation") def resend_invitation(portfolio_id, portfolio_token): invite = PortfolioInvitations.resend(g.current_user, portfolio_token) - send_invite_email(g.current_user.full_name, invite.token, invite.email) + send_portfolio_invitation(invite.email, g.current_user.full_name, invite.token) flash("resend_portfolio_invitation", user_name=invite.user_name) return redirect( url_for( @@ -61,3 +64,38 @@ def resend_invitation(portfolio_id, portfolio_token): _anchor="portfolio-members", ) ) + + +@portfolios_bp.route("/portfolios//members/new", methods=["POST"]) +@user_can(Permissions.CREATE_PORTFOLIO_USERS, message="create new portfolio member") +def invite_member(portfolio_id): + portfolio = Portfolios.get(g.current_user, portfolio_id) + form = member_forms.NewForm(http_request.form) + + if form.validate(): + try: + invite = Portfolios.invite(portfolio, g.current_user, form.update_data) + send_portfolio_invitation( + invite.email, g.current_user.full_name, invite.token + ) + + flash( + "new_portfolio_member", user_name=invite.user_name, portfolio=portfolio + ) + + except AlreadyExistsError: + return render_template( + "error.html", message="There was an error processing your request." + ) + else: + pass + # TODO: flash error message + + return redirect( + url_for( + "portfolios.admin", + portfolio_id=portfolio_id, + fragment="portfolio-members", + _anchor="portfolio-members", + ) + ) diff --git a/atst/routes/portfolios/members.py b/atst/routes/portfolios/members.py deleted file mode 100644 index 2ba90d86..00000000 --- a/atst/routes/portfolios/members.py +++ /dev/null @@ -1,57 +0,0 @@ -from flask import render_template, request as http_request, g, redirect, url_for - -from . import portfolios_bp -from atst.domain.exceptions import AlreadyExistsError -from atst.domain.portfolios import Portfolios -import atst.forms.portfolio_member as member_forms -from atst.domain.authz.decorator import user_can_access_decorator as user_can -from atst.models.permissions import Permissions - -from atst.utils.flash import formatted_flash as flash -from atst.queue import queue - - -def send_portfolio_invitation(invitee_email, inviter_name, token): - body = render_template( - "emails/portfolio/invitation.txt", owner=inviter_name, token=token - ) - queue.send_mail( - [invitee_email], - "{} has invited you to a JEDI cloud portfolio".format(inviter_name), - body, - ) - - -@portfolios_bp.route("/portfolios//members/new", methods=["POST"]) -@user_can(Permissions.CREATE_PORTFOLIO_USERS, message="create new portfolio member") -def create_member(portfolio_id): - portfolio = Portfolios.get(g.current_user, portfolio_id) - form = member_forms.NewForm(http_request.form) - - if form.validate(): - try: - invite = Portfolios.invite(portfolio, g.current_user, form.update_data) - send_portfolio_invitation( - invite.email, g.current_user.full_name, invite.token - ) - - flash( - "new_portfolio_member", user_name=invite.user_name, portfolio=portfolio - ) - - except AlreadyExistsError: - return render_template( - "error.html", message="There was an error processing your request." - ) - else: - pass - # TODO: flash error message - - return redirect( - url_for( - "portfolios.admin", - portfolio_id=portfolio_id, - fragment="portfolio-members", - _anchor="portfolio-members", - ) - ) diff --git a/templates/fragments/admin/add_new_portfolio_member.html b/templates/fragments/admin/add_new_portfolio_member.html index 85db3a12..bd6f1fc5 100644 --- a/templates/fragments/admin/add_new_portfolio_member.html +++ b/templates/fragments/admin/add_new_portfolio_member.html @@ -78,6 +78,6 @@ {{ MultiStepModalForm( 'add-port-mem', member_form, - url_for("portfolios.create_member", portfolio_id=portfolio.id), + url_for("portfolios.invite_member", portfolio_id=portfolio.id), [step_one, step_two], ) }} diff --git a/tests/routes/portfolios/test_invitations.py b/tests/routes/portfolios/test_invitations.py index cbc5594f..1ec7e9d3 100644 --- a/tests/routes/portfolios/test_invitations.py +++ b/tests/routes/portfolios/test_invitations.py @@ -1,18 +1,12 @@ -import pytest import datetime from flask import url_for -from tests.factories import ( - UserFactory, - PortfolioFactory, - PortfolioRoleFactory, - PortfolioInvitationFactory, - TaskOrderFactory, -) from atst.domain.portfolios import Portfolios from atst.models import InvitationStatus, PortfolioRoleStatus -from atst.domain.users import Users from atst.domain.permission_sets import PermissionSets +from atst.queue import queue + +from tests.factories import * def test_existing_member_accepts_valid_invite(client, user_session): @@ -92,7 +86,7 @@ def test_user_who_has_not_accepted_portfolio_invite_cannot_view(client, user_ses # create user in portfolio with invitation user_session(portfolio.owner) response = client.post( - url_for("portfolios.create_member", portfolio_id=portfolio.id), + url_for("portfolios.invite_member", portfolio_id=portfolio.id), data=user.to_dictionary(), ) @@ -263,3 +257,55 @@ def test_existing_member_invite_resent_to_email_submitted_in_form( assert user.email != "example@example.com" assert send_mail_job.func.__func__.__name__ == "_send_mail" assert send_mail_job.args[0] == ["example@example.com"] + + +_DEFAULT_PERMS_FORM_DATA = { + "permission_sets-perms_app_mgmt": PermissionSets.VIEW_PORTFOLIO_APPLICATION_MANAGEMENT, + "permission_sets-perms_funding": PermissionSets.VIEW_PORTFOLIO_FUNDING, + "permission_sets-perms_reporting": PermissionSets.VIEW_PORTFOLIO_REPORTS, + "permission_sets-perms_portfolio_mgmt": PermissionSets.VIEW_PORTFOLIO_ADMIN, +} + + +def test_user_with_permission_has_add_member_link(client, user_session): + portfolio = PortfolioFactory.create() + user_session(portfolio.owner) + response = client.get(url_for("portfolios.admin", portfolio_id=portfolio.id)) + assert response.status_code == 200 + assert ( + url_for("portfolios.invite_member", portfolio_id=portfolio.id).encode() + in response.data + ) + + +def test_invite_member(client, user_session, session): + user_data = UserFactory.dictionary() + portfolio = PortfolioFactory.create() + user_session(portfolio.owner) + queue_length = len(queue.get_queue()) + + response = client.post( + url_for("portfolios.invite_member", portfolio_id=portfolio.id), + data={ + "user_data-dod_id": user_data.get("dod_id"), + "user_data-first_name": user_data.get("first_name"), + "user_data-last_name": user_data.get("last_name"), + "user_data-email": user_data.get("email"), + **_DEFAULT_PERMS_FORM_DATA, + }, + follow_redirects=True, + ) + + assert response.status_code == 200 + full_name = "{} {}".format(user_data.get("first_name"), user_data.get("last_name")) + assert full_name in response.data.decode() + + invitation = ( + session.query(PortfolioInvitation) + .filter_by(dod_id=user_data.get("dod_id")) + .one() + ) + assert invitation.role.portfolio == portfolio + + assert len(queue.get_queue()) == queue_length + 1 + assert len(invitation.role.permission_sets) == 5 diff --git a/tests/routes/portfolios/test_members.py b/tests/routes/portfolios/test_members.py deleted file mode 100644 index cd046b6a..00000000 --- a/tests/routes/portfolios/test_members.py +++ /dev/null @@ -1,58 +0,0 @@ -from flask import url_for - -from atst.domain.permission_sets import PermissionSets -from atst.models import PortfolioInvitation -from atst.queue import queue - -from tests.factories import UserFactory, PortfolioFactory - -_DEFAULT_PERMS_FORM_DATA = { - "permission_sets-perms_app_mgmt": PermissionSets.VIEW_PORTFOLIO_APPLICATION_MANAGEMENT, - "permission_sets-perms_funding": PermissionSets.VIEW_PORTFOLIO_FUNDING, - "permission_sets-perms_reporting": PermissionSets.VIEW_PORTFOLIO_REPORTS, - "permission_sets-perms_portfolio_mgmt": PermissionSets.VIEW_PORTFOLIO_ADMIN, -} - - -def test_user_with_permission_has_add_member_link(client, user_session): - portfolio = PortfolioFactory.create() - user_session(portfolio.owner) - response = client.get(url_for("portfolios.admin", portfolio_id=portfolio.id)) - assert response.status_code == 200 - assert ( - url_for("portfolios.create_member", portfolio_id=portfolio.id).encode() - in response.data - ) - - -def test_create_member(client, user_session, session): - user_data = UserFactory.dictionary() - portfolio = PortfolioFactory.create() - user_session(portfolio.owner) - queue_length = len(queue.get_queue()) - - response = client.post( - url_for("portfolios.create_member", portfolio_id=portfolio.id), - data={ - "user_data-dod_id": user_data.get("dod_id"), - "user_data-first_name": user_data.get("first_name"), - "user_data-last_name": user_data.get("last_name"), - "user_data-email": user_data.get("email"), - **_DEFAULT_PERMS_FORM_DATA, - }, - follow_redirects=True, - ) - - assert response.status_code == 200 - full_name = "{} {}".format(user_data.get("first_name"), user_data.get("last_name")) - assert full_name in response.data.decode() - - invitation = ( - session.query(PortfolioInvitation) - .filter_by(dod_id=user_data.get("dod_id")) - .one() - ) - assert invitation.role.portfolio == portfolio - - assert len(queue.get_queue()) == queue_length + 1 - assert len(invitation.role.permission_sets) == 5 diff --git a/tests/test_access.py b/tests/test_access.py index e5d4c528..a551de9b 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -197,14 +197,14 @@ def test_applications_update_team_env_roles(post_url_assert_status): post_url_assert_status(rando, url, 404) -# portfolios.create_member -def test_portfolios_create_member_access(post_url_assert_status): +# portfolios.invite_member +def test_portfolios_invite_member_access(post_url_assert_status): ccpo = user_with(PermissionSets.EDIT_PORTFOLIO_ADMIN) owner = user_with() rando = user_with() portfolio = PortfolioFactory.create(owner=owner) - url = url_for("portfolios.create_member", portfolio_id=portfolio.id) + url = url_for("portfolios.invite_member", portfolio_id=portfolio.id) post_url_assert_status(ccpo, url, 302) post_url_assert_status(owner, url, 302) post_url_assert_status(rando, url, 404)