From 97fff41af514efc6a7dfa84d6f9ca5ca5b48457c Mon Sep 17 00:00:00 2001 From: Kiran Jonnalagadda Date: Tue, 26 Mar 2024 17:01:37 +0530 Subject: [PATCH] Add member-only visibility for project updates (#1988) --------- Co-authored-by: Amogh M Aradhya Co-authored-by: Zainab Bawa Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- Makefile | 2 +- funnel/forms/update.py | 25 +- funnel/models/__init__.py | 3 +- funnel/models/__init__.pyi | 7 +- funnel/models/account.py | 56 ++--- funnel/models/account_membership.py | 15 +- funnel/models/comment.py | 36 +-- funnel/models/contact_exchange.py | 24 +- funnel/models/notification.py | 41 ++- funnel/models/notification_types.py | 60 +++-- funnel/models/project.py | 65 +++-- funnel/models/proposal.py | 30 ++- funnel/models/saved.py | 15 +- funnel/models/sync_ticket.py | 25 +- funnel/models/update.py | 142 +++++------ funnel/templates/js/update.js.jinja2 | 5 +- .../notifications/layout_web.html.jinja2 | 3 +- .../project_starting_web.html.jinja2 | 1 - ...inja2 => project_update_email.html.jinja2} | 0 .../project_update_web.html.jinja2 | 40 +++ .../proposal_received_web.html.jinja2 | 1 - .../notifications/update_new_web.html.jinja2 | 14 -- .../user_password_set_web.html.jinja2 | 1 - funnel/views/mixins.py | 3 +- funnel/views/notifications/mixins.py | 18 ++ .../notifications/update_notification.py | 59 ++++- funnel/views/update.py | 15 +- ...20b31eb6_update_visibility_state_change.py | 50 ++++ .../bd377f7c3b3b_update_notification_data.py | 95 +++++++ sample.env | 3 +- tests/conftest.py | 4 +- tests/features/update/update_members.feature | 0 .../update/update_participants.feature | 4 + tests/features/update/update_pinned.feature | 0 tests/features/update/update_public.feature | 57 +++++ tests/unit/models/account_membership_test.py | 37 +++ tests/unit/models/merge_notification_test.py | 4 +- tests/unit/models/models_utils_test.py | 2 +- tests/unit/models/notification_test.py | 38 ++- tests/unit/models/update_test.py | 233 ++++++++++++++++++ tests/unit/views/notification_test.py | 15 +- 41 files changed, 925 insertions(+), 323 deletions(-) rename funnel/templates/notifications/{update_new_email.html.jinja2 => project_update_email.html.jinja2} (100%) create mode 100644 funnel/templates/notifications/project_update_web.html.jinja2 delete mode 100644 funnel/templates/notifications/update_new_web.html.jinja2 create mode 100644 migrations/versions/0afd20b31eb6_update_visibility_state_change.py create mode 100644 migrations/versions/bd377f7c3b3b_update_notification_data.py create mode 100644 tests/features/update/update_members.feature create mode 100644 tests/features/update/update_participants.feature create mode 100644 tests/features/update/update_pinned.feature create mode 100644 tests/features/update/update_public.feature create mode 100644 tests/unit/models/account_membership_test.py create mode 100644 tests/unit/models/update_test.py diff --git a/Makefile b/Makefile index 22d701d81..2b6d6e583 100644 --- a/Makefile +++ b/Makefile @@ -136,7 +136,7 @@ initpy-loginproviders: initpy-transports: # Do not auto-gen funnel/transports/__init__.py, only sub-packages mkinit --inplace --relative --black funnel/transports/email - mkinit --inplace --relative --black funnel/transports/sms + mkinit --inplace --relative --black funnel/transports/sms isort funnel/transports/*/__init__.py black funnel/transports/*/__init__.py diff --git a/funnel/forms/update.py b/funnel/forms/update.py index 9fcb4b2cd..6a299eaba 100644 --- a/funnel/forms/update.py +++ b/funnel/forms/update.py @@ -4,7 +4,7 @@ from baseframe import __, forms -from ..models import Update +from ..models import VISIBILITY_STATE, Update __all__ = ['UpdateForm', 'UpdatePinForm'] @@ -23,11 +23,24 @@ class UpdateForm(forms.Form): validators=[forms.validators.DataRequired()], description=__("Markdown formatting is supported"), ) - is_pinned = forms.BooleanField( - __("Pin this update above other updates"), default=False - ) - is_restricted = forms.BooleanField( - __("Limit access to current participants only"), default=False + visibility = forms.RadioField( + __("Who gets this update?"), + description=__("This can’t be changed after publishing the update"), + default=VISIBILITY_STATE[VISIBILITY_STATE.PUBLIC].name, + choices=[ + ( + VISIBILITY_STATE[VISIBILITY_STATE.PUBLIC].name, + __("Public; account followers will be notified"), + ), + ( + VISIBILITY_STATE[VISIBILITY_STATE.MEMBERS].name, + __("Only account members"), + ), + ( + VISIBILITY_STATE[VISIBILITY_STATE.PARTICIPANTS].name, + __("Only project participants"), + ), + ], ) diff --git a/funnel/models/__init__.py b/funnel/models/__init__.py index 1331e1e60..90e801c59 100644 --- a/funnel/models/__init__.py +++ b/funnel/models/__init__.py @@ -98,7 +98,6 @@ "ModelUrlProtocol", "ModelUuidProtocol", "NewCommentNotification", - "NewUpdateNotification", "NoIdMixin", "Notification", "NotificationFor", @@ -131,6 +130,7 @@ "ProjectSponsorMembership", "ProjectStartingNotification", "ProjectTomorrowNotification", + "ProjectUpdateNotification", "Proposal", "ProposalLabelProxy", "ProposalLabelProxyWrapper", @@ -171,6 +171,7 @@ "UrlType", "User", "UuidMixin", + "VISIBILITY_STATE", "Venue", "VenueRoom", "VideoError", diff --git a/funnel/models/__init__.pyi b/funnel/models/__init__.pyi index 98fd7c2bd..6752aa5cc 100644 --- a/funnel/models/__init__.pyi +++ b/funnel/models/__init__.pyi @@ -172,13 +172,13 @@ from .notification_types import ( CommentReplyNotification, CommentReportReceivedNotification, NewCommentNotification, - NewUpdateNotification, OrganizationAdminMembershipNotification, OrganizationAdminMembershipRevokedNotification, ProjectCrewMembershipNotification, ProjectCrewMembershipRevokedNotification, ProjectStartingNotification, ProjectTomorrowNotification, + ProjectUpdateNotification, ProposalReceivedNotification, ProposalSubmittedNotification, RegistrationCancellationNotification, @@ -229,7 +229,7 @@ from .typing import ( ModelUrlProtocol, ModelUuidProtocol, ) -from .update import Update +from .update import VISIBILITY_STATE, Update from .utils import ( AccountAndAnchor, IncompleteUserMigrationError, @@ -323,7 +323,6 @@ __all__ = [ "ModelUrlProtocol", "ModelUuidProtocol", "NewCommentNotification", - "NewUpdateNotification", "NoIdMixin", "Notification", "NotificationFor", @@ -356,6 +355,7 @@ __all__ = [ "ProjectSponsorMembership", "ProjectStartingNotification", "ProjectTomorrowNotification", + "ProjectUpdateNotification", "Proposal", "ProposalLabelProxy", "ProposalLabelProxyWrapper", @@ -396,6 +396,7 @@ __all__ = [ "UrlType", "User", "UuidMixin", + "VISIBILITY_STATE", "Venue", "VenueRoom", "VideoError", diff --git a/funnel/models/account.py b/funnel/models/account.py index eb1b303c4..d5b5b26d1 100644 --- a/funnel/models/account.py +++ b/funnel/models/account.py @@ -35,13 +35,13 @@ from baseframe import __ from coaster.sqlalchemy import ( DynamicAssociationProxy, - LazyRoleSet, RoleMixin, StateManager, add_primary_relationship, auto_init_default, failsafe_add, immutable, + role_check, with_roles, ) from coaster.utils import LabeledEnum, newsecret, require_one_of, utcnow @@ -281,9 +281,6 @@ class Account(UuidMixin, BaseMixin[int, 'Account'], Model): ImgeeType, sa.CheckConstraint("banner_image_url <> ''"), nullable=True ) - # These two flags are read-only. There is no provision for writing to them within - # the app: - #: Protected accounts cannot be deleted is_protected: Mapped[bool] = with_roles( immutable(sa_orm.mapped_column(default=False)), @@ -380,17 +377,20 @@ class Account(UuidMixin, BaseMixin[int, 'Account'], Model): order_by=lambda: AccountMembership.granted_at.asc(), viewonly=True, ), - grants_via={'member': {'admin', 'owner'}}, + grants_via={'member': {'admin', 'member'}}, ) - active_owner_memberships: DynamicMapped[AccountMembership] = relationship( - lazy='dynamic', - primaryjoin=lambda: sa.and_( - sa_orm.remote(AccountMembership.account_id) == Account.id, - AccountMembership.is_active, - AccountMembership.is_owner.is_(True), + active_owner_memberships: DynamicMapped[AccountMembership] = with_roles( + relationship( + lazy='dynamic', + primaryjoin=lambda: sa.and_( + sa_orm.remote(AccountMembership.account_id) == Account.id, + AccountMembership.is_active, + AccountMembership.is_owner.is_(True), + ), + viewonly=True, ), - viewonly=True, + grants_via={'member': {'owner', 'admin', 'member'}}, ) active_invitations: DynamicMapped[AccountMembership] = relationship( @@ -925,6 +925,7 @@ def ticket_followers(self) -> Query[Account]: 'polymorphic_on': type_, # When querying the Account model, cast automatically to all subclasses 'with_polymorphic': '*', + # Store a version id in this column to prevent edits to obsolete data 'version_id_col': revisionid, } @@ -1043,14 +1044,12 @@ def pickername(self) -> str: with_roles(pickername, read={'all'}) - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - """Identify roles for the given actor.""" - roles = super().roles_for(actor, anchors) - if self.profile_state.ACTIVE_AND_PUBLIC: - roles.add('reader') - return roles + @role_check('reader') + def has_reader_role( + self, _actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Grant 'reader' role to all if the profile state is active and public.""" + return bool(self.profile_state.ACTIVE_AND_PUBLIC) @cached_property def verified_contact_count(self) -> int: @@ -1394,16 +1393,19 @@ def default_email( return None @property - def _self_is_owner_and_admin_of_self(self) -> Account: + def _self_is_owner_of_self(self) -> Account | None: """ - Return self. + Return self in a user account. Helper method for :meth:`roles_for` and :meth:`actors_with` to assert that the user is owner and admin of their own account. """ - return self + return self if self.is_user_profile else None - with_roles(_self_is_owner_and_admin_of_self, grants={'owner', 'admin'}) + with_roles( + _self_is_owner_of_self, + grants={'follower', 'member', 'admin', 'owner'}, + ) def organizations_as_owner_ids(self) -> list[int]: """ @@ -2678,10 +2680,8 @@ class AccountExternalId(BaseMixin[int, Account], Model): # FIXME: change to sa.Unicode service: Mapped[str] = sa_orm.mapped_column(sa.UnicodeText, nullable=False) #: Unique user id as per external service, used for identifying related accounts - # FIXME: change to sa.Unicode - userid: Mapped[str] = sa_orm.mapped_column( - sa.UnicodeText, nullable=False - ) # Unique id (or obsolete OpenID) + # FIXME: change to sa.Unicode (uses UnicodeText for obsolete OpenID support) + userid: Mapped[str] = sa_orm.mapped_column(sa.UnicodeText, nullable=False) #: Optional public-facing username on the external service # FIXME: change to sa.Unicode. LinkedIn once used full URLs username: Mapped[str | None] = sa_orm.mapped_column(sa.UnicodeText, nullable=True) diff --git a/funnel/models/account_membership.py b/funnel/models/account_membership.py index f1aefb8b7..f188f22ac 100644 --- a/funnel/models/account_membership.py +++ b/funnel/models/account_membership.py @@ -15,13 +15,11 @@ class AccountMembership(ImmutableMembershipMixin, Model): """ - An account can be a member of another account as an owner, admin or follower. + An account can be an owner, admin, member or follower of another account. - Owners can manage other administrators. + Owners can manage other owners, admins and members, but not followers. - TODO: This model may introduce non-admin memberships in a future iteration by - replacing :attr:`is_owner` with :attr:`member_level` or distinct role flags as in - :class:`ProjectMembership`. + TODO: Distinct flags for is_member, is_follower and is_admin. """ __tablename__ = 'account_membership' @@ -76,7 +74,7 @@ class AccountMembership(ImmutableMembershipMixin, Model): 'related': {'urls', 'uuid_b58', 'offered_roles', 'is_owner'}, } - #: Organization that this membership is being granted on + #: Account that this membership is being granted on account_id: Mapped[int] = sa_orm.mapped_column( sa.ForeignKey('account.id', ondelete='CASCADE'), default=None, @@ -96,7 +94,10 @@ class AccountMembership(ImmutableMembershipMixin, Model): @cached_property def offered_roles(self) -> set[str]: """Roles offered by this membership record.""" - roles = {'admin'} + # TODO: is_member and is_admin will be distinct flags in the future, with the + # base role set to `follower` only. is_owner will remain, but if it's set, then + # is_admin must also be set (enforced with a check constraint) + roles = {'follower', 'member', 'admin'} if self.is_owner: roles.add('owner') return roles diff --git a/funnel/models/comment.py b/funnel/models/comment.py index 07e188a35..5534cbede 100644 --- a/funnel/models/comment.py +++ b/funnel/models/comment.py @@ -9,7 +9,13 @@ from werkzeug.utils import cached_property from baseframe import _, __ -from coaster.sqlalchemy import LazyRoleSet, RoleAccessProxy, StateManager, with_roles +from coaster.sqlalchemy import ( + LazyRoleSet, + RoleAccessProxy, + StateManager, + role_check, + with_roles, +) from coaster.utils import LabeledEnum from .account import ( @@ -204,14 +210,14 @@ def last_comment(self) -> Comment | None: with_roles(last_comment, read={'all'}, datasets={'primary'}) - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - parent_roles = self.parent.roles_for(actor, anchors) - if 'participant' in parent_roles or 'commenter' in parent_roles: - roles.add('parent_participant') - return roles + @role_check('parent_participant') + def has_parent_participant_role( + self, actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Confirm if the actor is a participant in the parent object.""" + return (parent := self.parent) is not None and parent.roles_for(actor).has_any( + {'participant', 'commenter'} + ) @with_roles(call={'all'}) @state.requires(state.NOT_DISABLED) @@ -521,12 +527,12 @@ def was_reviewed_by(self, account: Account) -> bool: CommentModeratorReport.reported_by == account, ).notempty() - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - roles.add('reader') - return roles + @role_check('reader') + def has_reader_role( + self, _actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Everyone is always a reader (for now).""" + return True add_search_trigger(Comment, 'search_vector') diff --git a/funnel/models/contact_exchange.py b/funnel/models/contact_exchange.py index 92c7cbd28..ccbe9bf0d 100644 --- a/funnel/models/contact_exchange.py +++ b/funnel/models/contact_exchange.py @@ -2,7 +2,7 @@ from __future__ import annotations -from collections.abc import Collection, Sequence +from collections.abc import Collection from dataclasses import dataclass from datetime import date as date_type, datetime from itertools import groupby @@ -12,7 +12,7 @@ from pytz import BaseTzInfo, timezone from sqlalchemy.ext.associationproxy import association_proxy -from coaster.sqlalchemy import LazyRoleSet +from coaster.sqlalchemy import with_roles from coaster.utils import uuid_to_base58 from .account import Account @@ -62,7 +62,9 @@ class ContactExchange(TimestampMixin, RoleMixin, Model): account_id: Mapped[int] = sa_orm.mapped_column( sa.ForeignKey('account.id', ondelete='CASCADE'), primary_key=True, default=None ) - account: Mapped[Account] = relationship(back_populates='scanned_contacts') + account: Mapped[Account] = with_roles( + relationship(back_populates='scanned_contacts'), grants={'owner'} + ) #: Participant whose contact was scanned ticket_participant_id: Mapped[int] = sa_orm.mapped_column( sa.ForeignKey('ticket_participant.id', ondelete='CASCADE'), @@ -70,8 +72,9 @@ class ContactExchange(TimestampMixin, RoleMixin, Model): default=None, index=True, ) - ticket_participant: Mapped[TicketParticipant] = relationship( - back_populates='scanned_contacts' + ticket_participant: Mapped[TicketParticipant] = with_roles( + relationship(back_populates='scanned_contacts'), + grants_via={'participant': {'subject'}}, ) #: Datetime at which the scan happened scanned_at: Mapped[datetime] = sa_orm.mapped_column( @@ -101,17 +104,6 @@ class ContactExchange(TimestampMixin, RoleMixin, Model): 'subject': {'read': {'account', 'ticket_participant', 'scanned_at'}}, } - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - if actor is not None: - if actor == self.account: - roles.add('owner') - if actor == self.ticket_participant.participant: - roles.add('subject') - return roles - @classmethod def migrate_account(cls, old_account: Account, new_account: Account) -> None: """Migrate one account's data to another when merging accounts.""" diff --git a/funnel/models/notification.py b/funnel/models/notification.py index d5128a31e..157bb3314 100644 --- a/funnel/models/notification.py +++ b/funnel/models/notification.py @@ -50,7 +50,7 @@ now queues a third series of background workers, for each of the supported transports if at least one recipient in that batch wants to use that transport. -6. A separate render view class named RenderNewUpdateNotification contains methods named +6. A separate render view class named RenderNotification contains methods named like `web`, `email`, `sms` and others. These are expected to return a rendered message. The `web` render is used for the notification feed page on the website. @@ -283,7 +283,7 @@ class NotificationType(Generic[_D, _F], Protocol): eventid_b58: str document: _D document_uuid: UUID - fragment: _F | None + fragment: _F fragment_uuid: UUID | None created_at: datetime created_by_id: int | None @@ -344,19 +344,22 @@ class Notification(NoIdMixin, Model, Generic[_D, _F]): #: another type (auto-populated from subclass's `shadow=` parameter) pref_type: ClassVar[str] = '' - #: Document model, must be specified in subclasses + #: Document model, auto-populated from generic arg to Notification base class document_model: ClassVar[type[ModelUuidProtocol]] #: SQL table name for document type, auto-populated from the document model document_type: ClassVar[str] - #: Fragment model, optional for subclasses - fragment_model: ClassVar[type[ModelUuidProtocol] | None] = None + #: Fragment model, auto-populated from generic arg to Notification base class + fragment_model: ClassVar[type[ModelUuidProtocol] | None] #: SQL table name for fragment type, auto-populated from the fragment model fragment_type: ClassVar[str | None] #: Roles to send notifications to. Roles must be in order of priority for situations - #: where a user has more than one role on the document. - roles: ClassVar[Sequence[str]] = [] + #: where a user has more than one role on the document. The notification can + #: customize target roles based on the document or fragment's properties + @property + def dispatch_roles(self) -> Sequence[str]: + return [] #: Exclude triggering actor from receiving notifications? Subclasses may override exclude_actor: ClassVar[bool] = False @@ -609,18 +612,19 @@ def document(self) -> _D: ) @cached_property - def fragment(self) -> _F | None: + def fragment(self) -> _F: """ Retrieve the fragment within a document referenced by this Notification, if any. This assumes the underlying object won't disappear, as there is no SQL foreign key constraint enforcing a link. """ - if self.fragment_uuid and self.fragment_model: + if self.fragment_model is not None and self.fragment_uuid is not None: return cast( _F, self.fragment_model.query.filter_by(uuid=self.fragment_uuid).one() ) - return None + # TypeVar _F may be typed `None` in a subclass, but we can't type it so here + return None # type: ignore[return-value] @classmethod def renderer(cls, view: type[T]) -> type[T]: @@ -667,7 +671,7 @@ def dispatch(self) -> Generator[NotificationRecipient, None, None]: should override this method. """ for account, role in self.role_provider_obj.actors_with( - self.roles, with_role=True + self.dispatch_roles, with_role=True ): # If this notification requires that it not be sent to the actor that # triggered the notification, don't notify them. For example, a user who @@ -707,14 +711,6 @@ def dispatch(self) -> Generator[NotificationRecipient, None, None]: yield recipient -# Make :attr:`type_` available under the name `type`, but declare this outside the class -# (a) to avoid conflicts with the Python `type` global that is used for type-hinting, -# and (b) because SQLAlchemy >= 2.0.26 attempts to resolve annotations using the -# class-local namespace, which overrides the global `type` (as of this commit): -# https://github.com/sqlalchemy/sqlalchemy/commit/153f287b9949462ec29d66fc9b329d0144a6ca7c -Notification.type = sa_orm.synonym('type_') - - class PreviewNotification(NotificationType): """ Mimics a Notification subclass without instantiating it, for providing a preview. @@ -1242,7 +1238,7 @@ def role(self) -> str | None: """User's primary matching role for this notification.""" if self.document and self.recipient: roles = self.document.roles_for(self.recipient) - for role in self.notification.roles: + for role in self.notification.dispatch_roles: if role in roles: return role return None @@ -1278,8 +1274,9 @@ class NotificationPreferences(BaseMixin[int, Account], Model): grants={'owner'}, ) - # Notification type, corresponding to Notification.type (a class attribute there) - # notification_type = '' holds the veto switch to disable a transport entirely + # Notification type, corresponding to `Notification.cls_type` (a class attribute + # there). `notification_type = ''` holds the veto switch to disable a transport + # entirely notification_type: Mapped[str] = immutable(sa_orm.mapped_column()) by_email: Mapped[bool] = with_roles(sa_orm.mapped_column(), rw={'owner'}) diff --git a/funnel/models/notification_types.py b/funnel/models/notification_types.py index b5ac3901f..31c820fb9 100644 --- a/funnel/models/notification_types.py +++ b/funnel/models/notification_types.py @@ -1,5 +1,10 @@ """Notification types.""" +# Pyright complains that a property in the base class (for `roles`) has become a +# classvar in the subclass. Mypy does not. Silence Pyright here + +# pyright: reportAssignmentType=false + from __future__ import annotations from baseframe import __ @@ -18,7 +23,7 @@ __all__ = [ 'AccountPasswordNotification', - 'NewUpdateNotification', + 'ProjectUpdateNotification', 'CommentReportReceivedNotification', 'CommentReplyNotification', 'NewCommentNotification', @@ -77,7 +82,7 @@ class AccountPasswordNotification( description = __("For your safety, in case this was not authorized") exclude_actor = False - roles = ['owner'] + dispatch_roles = ['owner'] for_private_recipient = True @@ -93,7 +98,7 @@ class RegistrationConfirmationNotification( title = __("When I register for a project") description = __("This will prompt a calendar entry in Gmail and other apps") - roles = ['owner'] + dispatch_roles = ['owner'] exclude_actor = False # This is a notification to the actor for_private_recipient = True @@ -106,26 +111,41 @@ class RegistrationCancellationNotification( ): """Notification confirming cancelling registration to a project.""" - roles = ['owner'] + dispatch_roles = ['owner'] exclude_actor = False # This is a notification to the actor for_private_recipient = True allow_web = False -class NewUpdateNotification( - DocumentHasProject, Notification[Update, None], type='update_new' +class ProjectUpdateNotification( + DocumentHasAccount, Notification[Project, Update], type='project_update' ): - """Notifications of new updates.""" + """Notification of a new update in a project.""" category = notification_categories.participant - title = __("When a project posts an update") + title = __("When a project has an update") description = __( "Typically contains critical information such as video conference links" ) - roles = ['project_crew', 'project_participant', 'account_participant'] exclude_actor = False # Send to everyone including the actor + @property + def dispatch_roles(self) -> list[str]: + """Target roles based on Update visibility state.""" + # TODO: Use match/case matching here. If states use a Python Enum, Mypy will + # do an exhaustiveness check, so the closing RuntimeError is not needed. + # https://github.com/python/mypy/issues/6366 + visibility = self.fragment.visibility_state + if visibility.PUBLIC: + return ['project_crew', 'project_participant', 'account_follower'] + if visibility.PARTICIPANTS: + return ['project_crew', 'project_participant'] + if visibility.MEMBERS: + return ['project_crew', 'project_participant', 'account_member'] + + raise RuntimeError("Unknown update visibility state") + class ProposalSubmittedNotification( DocumentHasProject, Notification[Proposal, None], type='proposal_submitted' @@ -136,7 +156,7 @@ class ProposalSubmittedNotification( title = __("When I submit a proposal") description = __("Confirmation for your records") - roles = ['creator'] + dispatch_roles = ['creator'] exclude_actor = False # This notification is for the actor # Email is typically fine. Messengers may be too noisy @@ -173,7 +193,7 @@ class ProjectTomorrowNotification( ): """Notification of an in-person session the next day.""" - roles = ['project_crew', 'project_participant'] + dispatch_roles = ['project_crew', 'project_participant'] # This is a notification triggered without an actor @@ -187,7 +207,7 @@ class NewCommentNotification(Notification[Commentset, Comment], type='comment_ne title = __("When there is a new comment on something I’m involved in") exclude_actor = True - roles = ['replied_to_commenter', 'document_subscriber'] + dispatch_roles = ['replied_to_commenter', 'document_subscriber'] class CommentReplyNotification(Notification[Comment, Comment], type='comment_reply'): @@ -199,7 +219,7 @@ class CommentReplyNotification(Notification[Comment, Comment], type='comment_rep # document_model = Parent comment (being replied to) # fragment_model = Child comment (the reply that triggered notification) - roles = ['replied_to_commenter'] + dispatch_roles = ['replied_to_commenter'] # --- Project crew notifications ------------------------------------------------------- @@ -216,7 +236,7 @@ class ProjectCrewMembershipNotification( title = __("When a project crew member is added or removed") description = __("Crew members have access to the project’s settings and data") - roles = ['member', 'project_crew'] + dispatch_roles = ['member', 'project_crew'] exclude_actor = True # Alerts other users of actor's actions; too noisy for actor @@ -228,7 +248,7 @@ class ProjectCrewMembershipRevokedNotification( ): """Notification of being removed from crew membership (including role changes).""" - roles = ['member', 'project_crew'] + dispatch_roles = ['member', 'project_crew'] exclude_actor = True # Alerts other users of actor's actions; too noisy for actor @@ -240,7 +260,7 @@ class ProposalReceivedNotification( category = notification_categories.project_crew title = __("When my project receives a new proposal") - roles = ['project_editor'] + dispatch_roles = ['project_editor'] exclude_actor = True # Don't notify editor of proposal they submitted @@ -254,7 +274,7 @@ class RegistrationReceivedNotification( category = notification_categories.project_crew title = __("When someone registers for my project") - roles = ['project_promoter'] + dispatch_roles = ['project_promoter'] exclude_actor = True @@ -272,7 +292,7 @@ class OrganizationAdminMembershipNotification( title = __("When account admins change") description = __("Account admins control all projects under the account") - roles = ['member', 'account_admin'] + dispatch_roles = ['member', 'account_admin'] exclude_actor = True # Alerts other users of actor's actions; too noisy for actor @@ -284,7 +304,7 @@ class OrganizationAdminMembershipRevokedNotification( ): """Notification of being granted admin membership (including role changes).""" - roles = ['member', 'account_admin'] + dispatch_roles = ['member', 'account_admin'] exclude_actor = True # Alerts other users of actor's actions; too noisy for actor @@ -299,4 +319,4 @@ class CommentReportReceivedNotification( category = notification_categories.site_admin title = __("When a comment is reported as spam") - roles = ['comment_moderator'] + dispatch_roles = ['comment_moderator'] diff --git a/funnel/models/project.py b/funnel/models/project.py index 3f110ddfe..409364db5 100644 --- a/funnel/models/project.py +++ b/funnel/models/project.py @@ -5,7 +5,7 @@ from __future__ import annotations from collections import OrderedDict, defaultdict -from collections.abc import Sequence +from collections.abc import Iterable, Sequence from datetime import datetime, timedelta from enum import ReprEnum from typing import TYPE_CHECKING, Any, Literal, Self, cast, overload @@ -20,9 +20,9 @@ from baseframe import __, localize_timezone from coaster.sqlalchemy import ( DynamicAssociationProxy, - LazyRoleSet, ManagedState, StateManager, + role_check, with_roles, ) from coaster.utils import LabeledEnum, buid, utcnow @@ -102,11 +102,12 @@ class Project(UuidMixin, BaseScopedNameMixin[int, Account], Model): account: Mapped[Account] = with_roles( relationship(foreign_keys=[account_id], back_populates='projects'), read={'all'}, - # If account grants an 'admin' role, make it 'account_admin' here + # Remap account roles for use in project grants_via={ None: { + 'owner': 'account_owner', 'admin': 'account_admin', - 'follower': 'account_participant', + 'follower': 'account_follower', 'member': 'account_member', } }, @@ -686,6 +687,31 @@ def __format__(self, format_spec: str) -> str: return self.joined_title return format(self.joined_title, format_spec) + @role_check('member_participant') + def has_member_participant_role( + self, actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Confirm if the actor is both a participant and an account member.""" + if actor is None: + return False + roles = self.roles_for(actor) + if 'participant' in roles and 'account_member' in roles: + return True + return False + + @has_member_participant_role.iterable + def _(self) -> Iterable[Account]: + """All participants who are also account members.""" + # TODO: This iterable causes a loop of SQL queries. It will be far more + # efficient to SQL JOIN the data sources once they are single-table sources. + # Rsvp needs to merge into ProjectMembership, and AccountMembership needs to + # replace the hacky "membership project" data source. + return ( + account + for account in self.actors_with({'participant'}) + if 'account_member' in self.roles_for(account) + ) + @with_roles(call={'editor'}) @cfp_state.transition( cfp_state.OPENABLE, @@ -767,7 +793,7 @@ def title_suffix(self) -> str: with_roles(title_suffix, read={'all'}) @property - def title_parts(self) -> list[str]: + def title_parts(self) -> tuple[str] | tuple[str, str]: """ Return the hierarchy of titles of this project. @@ -780,9 +806,9 @@ def title_parts(self) -> list[str]: """ if self.short_title == self.title: # Project title does not derive from account title, so use both - return [self.account.title, self.title] + return (self.account.title, self.title) # Project title extends account title, so account title is not needed - return [self.title] + return (self.title,) with_roles(title_parts, read={'all'}) @@ -928,21 +954,19 @@ def rsvp_for(self, account: Account | None, create: bool = False) -> Rsvp | None return Rsvp.get_for(self, account, create) def rsvps_with(self, state: RsvpStateEnum) -> Query[Rsvp]: + # pylint: disable=protected-access return self.rsvps.join(Account).filter( - Account.state.ACTIVE, - Rsvp._state == state, # pylint: disable=protected-access + Account.state.ACTIVE, Rsvp._state == state ) def rsvp_counts(self) -> dict[str, int]: + # pylint: disable=protected-access return { row[0]: row[1] - for row in db.session.query( - Rsvp._state, # pylint: disable=protected-access - sa.func.count(Rsvp._state), # pylint: disable=protected-access - ) + for row in db.session.query(Rsvp._state, sa.func.count(Rsvp._state)) .join(Account) .filter(Account.state.ACTIVE, Rsvp.project == self) - .group_by(Rsvp._state) # pylint: disable=protected-access + .group_by(Rsvp._state) .all() } @@ -959,13 +983,12 @@ def update_schedule_timestamps(self) -> None: self.start_at = self.schedule_start_at self.end_at = self.schedule_end_at - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - # https://github.com/hasgeek/funnel/pull/220#discussion_r168718052 - roles.add('reader') - return roles + @role_check('reader') + def has_reader_role( + self, _actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Unconditionally grant reader role (for now).""" + return True def is_safe_to_delete(self) -> bool: """Return True if project has no proposals.""" diff --git a/funnel/models/proposal.py b/funnel/models/proposal.py index ad75bd4e1..63c9fb7d9 100644 --- a/funnel/models/proposal.py +++ b/funnel/models/proposal.py @@ -12,8 +12,8 @@ from baseframe.filters import preview from coaster.sqlalchemy import ( DynamicAssociationProxy, - LazyRoleSet, StateManager, + role_check, with_roles, ) from coaster.utils import LabeledEnum @@ -539,21 +539,19 @@ def has_sponsors(self) -> bool: sponsors = DynamicAssociationProxy[Account]('sponsor_memberships', 'member') - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - if self.state.DRAFT: - if 'reader' in roles: - # https://github.com/hasgeek/funnel/pull/220#discussion_r168724439 - roles.remove('reader') - else: - roles.add('reader') - - if roles.has_any(('project_participant', 'submitter')): - roles.add('commenter') - - return roles + @role_check('reader') + def has_reader_role( + self, _actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Grant reader role if the proposal is not a draft.""" + return not self.state.DRAFT + + @role_check('commenter') + def has_commenter_role( + self, actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Grant 'commenter' role to any participant or submitter.""" + return self.roles_for(actor).has_any(('project_participant', 'submitter')) @classmethod def all_public(cls) -> Query[Self]: diff --git a/funnel/models/saved.py b/funnel/models/saved.py index 53a30fc91..441bf3273 100644 --- a/funnel/models/saved.py +++ b/funnel/models/saved.py @@ -2,10 +2,9 @@ from __future__ import annotations -from collections.abc import Sequence from datetime import datetime -from coaster.sqlalchemy import LazyRoleSet, with_roles +from coaster.sqlalchemy import with_roles from .account import Account from .base import Mapped, Model, NoIdMixin, db, relationship, sa, sa_orm @@ -68,7 +67,9 @@ class SavedSession(NoIdMixin, Model): nullable=False, primary_key=True, ) - account: Mapped[Account] = relationship(back_populates='saved_sessions') + account: Mapped[Account] = with_roles( + relationship(back_populates='saved_sessions'), grants={'owner'} + ) #: Session that was saved session_id: Mapped[int] = sa_orm.mapped_column( sa.ForeignKey('session.id', ondelete='CASCADE'), @@ -90,14 +91,6 @@ class SavedSession(NoIdMixin, Model): sa.UnicodeText, nullable=True ) - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - if actor is not None and actor == self.account: - roles.add('owner') - return roles - @classmethod def migrate_account(cls, old_account: Account, new_account: Account) -> None: """Migrate one account's data to another when merging accounts.""" diff --git a/funnel/models/sync_ticket.py b/funnel/models/sync_ticket.py index d07b1e313..d75c4f647 100644 --- a/funnel/models/sync_ticket.py +++ b/funnel/models/sync_ticket.py @@ -4,10 +4,10 @@ import base64 import os -from collections.abc import Iterable, Sequence +from collections.abc import Iterable from typing import TYPE_CHECKING, Any, Self -from coaster.sqlalchemy import LazyRoleSet, with_roles +from coaster.sqlalchemy import with_roles from .account import Account, AccountEmail from .base import ( @@ -258,8 +258,8 @@ class TicketParticipant( participant_id: Mapped[int | None] = sa_orm.mapped_column( sa.ForeignKey('account.id'), default=None, nullable=True ) - participant: Mapped[Account | None] = relationship( - back_populates='ticket_participants' + participant: Mapped[Account | None] = with_roles( + relationship(back_populates='ticket_participants'), grants={'member'} ) project_id: Mapped[int] = sa_orm.mapped_column( sa.ForeignKey('project.id'), default=None, nullable=False @@ -270,8 +270,9 @@ class TicketParticipant( grants_via={None: project_child_role_map}, ) - scanned_contacts: Mapped[ContactExchange] = relationship( - passive_deletes=True, back_populates='ticket_participant' + scanned_contacts: Mapped[ContactExchange] = with_roles( + relationship(passive_deletes=True, back_populates='ticket_participant'), + grants_via={'account': {'scanner'}}, ) ticket_events: Mapped[list[TicketEvent]] = relationship( @@ -295,18 +296,6 @@ class TicketParticipant( 'scanner': {'read': {'email'}}, } - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - if actor is not None: - if actor == self.participant: - roles.add('member') - cx = db.session.get(ContactExchange, (actor.id, self.id)) - if cx is not None: - roles.add('scanner') - return roles - @property def avatar(self) -> str: return ( diff --git a/funnel/models/update.py b/funnel/models/update.py index 4bd3a6db4..c03436d30 100644 --- a/funnel/models/update.py +++ b/funnel/models/update.py @@ -7,7 +7,7 @@ from typing import Any, Self from baseframe import __ -from coaster.sqlalchemy import LazyRoleSet, StateManager, auto_init_default, with_roles +from coaster.sqlalchemy import StateManager, auto_init_default, role_check, with_roles from coaster.utils import LabeledEnum from .account import Account @@ -31,7 +31,7 @@ ) from .project import Project -__all__ = ['Update'] +__all__ = ['Update', 'VISIBILITY_STATE'] class UPDATE_STATE(LabeledEnum): # noqa: N801 @@ -42,12 +42,15 @@ class UPDATE_STATE(LabeledEnum): # noqa: N801 class VISIBILITY_STATE(LabeledEnum): # noqa: N801 PUBLIC = (1, 'public', __("Public")) - RESTRICTED = (2, 'restricted', __("Restricted")) + PARTICIPANTS = (2, 'participants', __("Participants")) + MEMBERS = (3, 'members', __("Members")) class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): __tablename__ = 'update' + # FIXME: Why is this a state? There's no state change in the product design. + # It's a permanent subtype identifier _visibility_state: Mapped[int] = sa_orm.mapped_column( 'visibility_state', sa.SmallInteger, @@ -93,45 +96,30 @@ class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): datasets={'primary'}, grants_via={ None: { + 'reader': {'project_reader'}, # Project reader is NOT update reader 'editor': {'editor', 'project_editor'}, - 'participant': {'reader', 'project_participant'}, - 'crew': {'reader', 'project_crew'}, + 'participant': {'project_participant'}, + 'account_follower': {'account_follower'}, + 'account_member': {'account_member'}, + 'member_participant': {'member_participant'}, + 'crew': {'project_crew', 'reader'}, } }, ) parent: Mapped[Project] = sa_orm.synonym('project') - # Relationship to project that exists only when the Update is not restricted, for - # the purpose of inheriting the account_participant role. We do this because - # RoleMixin does not have a mechanism for conditional grant of roles. A relationship - # marked as `grants_via` will always grant the role unconditionally, so the only - # control at the moment is to make the relationship itself conditional. The affected - # mechanism is not `roles_for` but `actors_with`, which is currently not meant to be - # redefined in a subclass - _project_when_unrestricted: Mapped[Project] = with_roles( - relationship( - viewonly=True, - uselist=False, - primaryjoin=sa.and_( - project_id == Project.id, _visibility_state == VISIBILITY_STATE.PUBLIC - ), - ), - grants_via={None: {'account_participant': 'account_participant'}}, - ) - body, body_text, body_html = MarkdownCompositeDocument.create( 'body', nullable=False ) - #: Update number, for Project updates, assigned when the update is published + #: Update serial number, only assigned when the update is published number: Mapped[int | None] = with_roles( sa_orm.mapped_column(default=None), read={'all'} ) - #: Like pinned tweets. You can keep posting updates, - #: but might want to pin an update from a week ago. + #: Pin an update above future updates is_pinned: Mapped[bool] = with_roles( - sa_orm.mapped_column(default=False), read={'all'} + sa_orm.mapped_column(default=False), read={'all'}, write={'editor'} ) published_by_id: Mapped[int | None] = sa_orm.mapped_column( @@ -185,6 +173,8 @@ class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): 'body_text', weights={'name': 'A', 'title': 'A', 'body_text': 'B'}, regconfig='english', + # FIXME: Search preview will give partial access to Update.body even if the + # user does not have the necessary 'reader' role hltext=lambda: sa.func.concat_ws( visual_field_delimiter, Update.title, Update.body_html ), @@ -198,6 +188,8 @@ class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): 'read': {'name', 'title', 'urls'}, 'call': {'features', 'visibility_state', 'state', 'url_for'}, }, + 'project_crew': {'read': {'body'}}, + 'editor': {'write': {'title', 'body'}, 'read': {'body'}}, 'reader': {'read': {'body'}}, } @@ -213,10 +205,8 @@ class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): 'edited_at', 'created_by', 'is_pinned', - 'is_restricted', 'is_currently_restricted', - 'visibility_label', - 'state_label', + 'visibility', 'urls', 'uuid_b58', }, @@ -231,10 +221,8 @@ class Update(UuidMixin, BaseScopedIdNameMixin[int, Account], Model): 'edited_at', 'created_by', 'is_pinned', - 'is_restricted', 'is_currently_restricted', - 'visibility_label', - 'state_label', + 'visibility', 'urls', 'uuid_b58', }, @@ -249,17 +237,47 @@ def __repr__(self) -> str: """Represent :class:`Update` as a string.""" return f'' - @property - def visibility_label(self) -> str: - return self.visibility_state.label.title - - with_roles(visibility_label, read={'all'}) + @role_check('reader') + def has_reader_role( + self, actor: Account | None, _anchors: Sequence[Any] = () + ) -> bool: + """Check if the given actor is a reader based on the Update's visibility.""" + if not self.state.PUBLISHED: + # Update must be published to allow anyone other than crew to read + return False + if self.visibility_state.PUBLIC: + return True + roles = self.roles_for(actor) + if self.visibility_state.PARTICIPANTS: + return 'project_participant' in roles + if self.visibility_state.MEMBERS: + return 'account_member' in roles + + raise RuntimeError("This update has an unexpected state") + + # 'reader' is a non-enumerated role, like `all`, `auth` and `anon` @property - def state_label(self) -> str: - return self.state.label.title - - with_roles(state_label, read={'all'}) + def visibility(self) -> str: + """Return visibility state name.""" + return self.visibility_state.label.name + + @visibility.setter + def visibility(self, value: str) -> None: + """Set visibility state (interim until visibility as state is resolved).""" + # FIXME: Move to using an Enum so we don't reproduce the enumeration here + match value: + case 'public': + vstate = VISIBILITY_STATE.PUBLIC + case 'participants': + vstate = VISIBILITY_STATE.PARTICIPANTS + case 'members': + vstate = VISIBILITY_STATE.MEMBERS + case _: + raise ValueError("Unknown visibility state") + self._visibility_state = vstate # type: ignore[assignment] + + with_roles(visibility, read={'all'}, write={'editor'}) state.add_conditional_state( 'UNPUBLISHED', @@ -311,51 +329,17 @@ def delete(self, actor: Account) -> None: @with_roles(call={'editor'}) @state.transition(state.DELETED, state.DRAFT) - def undo_delete(self) -> None: + def undelete(self) -> None: self.deleted_by = None self.deleted_at = None - @with_roles(call={'editor'}) - @visibility_state.transition(visibility_state.RESTRICTED, visibility_state.PUBLIC) - def make_public(self) -> None: - pass - - @with_roles(call={'editor'}) - @visibility_state.transition(visibility_state.PUBLIC, visibility_state.RESTRICTED) - def make_restricted(self) -> None: - pass - - @property - def is_restricted(self) -> bool: - return bool(self.visibility_state.RESTRICTED) - - @is_restricted.setter - def is_restricted(self, value: bool) -> None: - if value and self.visibility_state.PUBLIC: - self.make_restricted() - elif not value and self.visibility_state.RESTRICTED: - self.make_public() - - with_roles(is_restricted, read={'all'}) - @property def is_currently_restricted(self) -> bool: - return self.is_restricted and not self.current_roles.reader + """Check if this update is not available for the current user.""" + return not self.current_roles.reader with_roles(is_currently_restricted, read={'all'}) - def roles_for( - self, actor: Account | None = None, anchors: Sequence = () - ) -> LazyRoleSet: - roles = super().roles_for(actor, anchors) - if not self.visibility_state.RESTRICTED: - # Everyone gets reader role when the post is not restricted. - # If it is, 'reader' must be mapped from 'participant' in the project, - # specified above in the grants_via annotation on project. - roles.add('reader') - - return roles - @classmethod def all_published_public(cls) -> Query[Self]: return cls.query.join(Project).filter( diff --git a/funnel/templates/js/update.js.jinja2 b/funnel/templates/js/update.js.jinja2 index e93572c62..7d5972905 100644 --- a/funnel/templates/js/update.js.jinja2 +++ b/funnel/templates/js/update.js.jinja2 @@ -26,7 +26,10 @@
  • {{ gettext('Update') }} #{{ update.number }}
  • {{ age }}
    • -
  • +
  • + +
    • +
  • diff --git a/funnel/templates/notifications/layout_web.html.jinja2 b/funnel/templates/notifications/layout_web.html.jinja2 index f48f2d314..e7a34dfaf 100644 --- a/funnel/templates/notifications/layout_web.html.jinja2 +++ b/funnel/templates/notifications/layout_web.html.jinja2 @@ -1,5 +1,4 @@ -{%- from "macros.html.jinja2" import faicon %} -{%- from "macros.html.jinja2" import useravatar %} +{%- from "macros.html.jinja2" import faicon, useravatar %} {%- block update %} {%- if view.has_current_access() %}
    diff --git a/funnel/templates/notifications/project_starting_web.html.jinja2 b/funnel/templates/notifications/project_starting_web.html.jinja2 index c197a74d1..31006414f 100644 --- a/funnel/templates/notifications/project_starting_web.html.jinja2 +++ b/funnel/templates/notifications/project_starting_web.html.jinja2 @@ -1,5 +1,4 @@ {% extends "notifications/layout_web.html.jinja2" %} -{%- from "macros.html.jinja2" import useravatar %} {%- block avatar %} {{ useravatar(view.project.account) }} diff --git a/funnel/templates/notifications/update_new_email.html.jinja2 b/funnel/templates/notifications/project_update_email.html.jinja2 similarity index 100% rename from funnel/templates/notifications/update_new_email.html.jinja2 rename to funnel/templates/notifications/project_update_email.html.jinja2 diff --git a/funnel/templates/notifications/project_update_web.html.jinja2 b/funnel/templates/notifications/project_update_web.html.jinja2 new file mode 100644 index 000000000..cf28fa818 --- /dev/null +++ b/funnel/templates/notifications/project_update_web.html.jinja2 @@ -0,0 +1,40 @@ +{% extends "notifications/layout_web.html.jinja2" %} + +{% block content %} + {% if view.is_rollup %} +

    + {%- trans project=view.update.project.joined_title, project_url=view.update.project.url_for() -%} + Updates in {{ project }}: + {%- endtrans %} +

    +
      + {%- for update in view.fragments %} +
    1. + {%- trans url=update.url_for(), title=update.title, age=update.published_at|age -%} + {{ title }} ({{ age }}) + {%- endtrans -%} +
      2. + {%- endfor %} +
    + {% else %} +

    + {%- trans actor=view.actor.pickername, project=view.update.project.joined_title, project_url=view.update.project.url_for() -%} + {{ actor }} posted an update in {{ project }}: + {%- endtrans %} +

    +

    + {{ view.update.title }} +

    +
    + {{ view.update.body }} +
    + {% endif %} +{% endblock content %} + +{%- block avatar %} + {%- if view.is_rollup -%} + {{ useravatar(view.project.account) }} + {%- else -%} + {{ useravatar(view.actor) }} + {%- endif -%} +{%- endblock avatar %} diff --git a/funnel/templates/notifications/proposal_received_web.html.jinja2 b/funnel/templates/notifications/proposal_received_web.html.jinja2 index a7932a686..9edb2e525 100644 --- a/funnel/templates/notifications/proposal_received_web.html.jinja2 +++ b/funnel/templates/notifications/proposal_received_web.html.jinja2 @@ -1,5 +1,4 @@ {% extends "notifications/layout_web.html.jinja2" %} -{%- from "macros.html.jinja2" import faicon %} {% block content %} {%- if view.is_rollup %} diff --git a/funnel/templates/notifications/update_new_web.html.jinja2 b/funnel/templates/notifications/update_new_web.html.jinja2 deleted file mode 100644 index c55f81be0..000000000 --- a/funnel/templates/notifications/update_new_web.html.jinja2 +++ /dev/null @@ -1,14 +0,0 @@ -{% extends "notifications/layout_web.html.jinja2" %} -{% block content %} -

    - {%- trans actor=view.actor.pickername, project=view.update.project.joined_title, project_url=view.update.project.url_for() -%} - {{ actor }} posted an update in {{ project }}: - {%- endtrans %} -

    -

    - {{ view.update.title }} -

    -
    - {{ view.update.body }} -
    -{% endblock content %} diff --git a/funnel/templates/notifications/user_password_set_web.html.jinja2 b/funnel/templates/notifications/user_password_set_web.html.jinja2 index cf8d23a31..241eee9dd 100644 --- a/funnel/templates/notifications/user_password_set_web.html.jinja2 +++ b/funnel/templates/notifications/user_password_set_web.html.jinja2 @@ -1,5 +1,4 @@ {%- extends "notifications/layout_web.html.jinja2" %} -{%- from "macros.html.jinja2" import faicon %} {%- block content -%}

    diff --git a/funnel/views/mixins.py b/funnel/views/mixins.py index 5794e3867..7be8d7e77 100644 --- a/funnel/views/mixins.py +++ b/funnel/views/mixins.py @@ -130,6 +130,7 @@ def post_init(self) -> None: self.account = self.obj.project.account +# FIXME: Make this a generic like ModelView class DraftViewProtoMixin: # These must be Any to avoid conflict with subclasses model: Any @@ -160,7 +161,7 @@ def get_draft_data( """ Return a tuple of draft data. - Contains the current draft revision and the formdata needed to initialize forms. + Contains the current draft revision and the data needed to initialize forms. """ draft = self.get_draft(obj) if draft is not None: diff --git a/funnel/views/notifications/mixins.py b/funnel/views/notifications/mixins.py index 335ab3068..11ade9e79 100644 --- a/funnel/views/notifications/mixins.py +++ b/funnel/views/notifications/mixins.py @@ -72,6 +72,15 @@ def project(self, project: Project) -> str: index = grapheme.safe_split_index(title, self.var_max_length - 1) return title[:index] + '…' + @SetVar + def project_title(self, project: Project) -> str: + """Set project title, truncated to fit the length limit.""" + title = project.title + if len(title) <= self.var_max_length: + return title + index = grapheme.safe_split_index(title, self.var_max_length - 1) + return title[:index] + '…' + @SetVar def account(self, account: Account) -> str: """Set account's display name, truncated to fit.""" @@ -84,6 +93,15 @@ def account(self, account: Account) -> str: index = grapheme.safe_split_index(title, self.var_max_length - 1) return title[:index] + '…' + @SetVar + def account_title(self, account: Account) -> str: + """Set account's title, truncated to fit.""" + title = account.title + if len(title) <= self.var_max_length: + return title + index = grapheme.safe_split_index(title, self.var_max_length - 1) + return title[:index] + '…' + # This will trigger cloning in SetVar.__set_name__ actor = user = organization = profile = account diff --git a/funnel/views/notifications/update_notification.py b/funnel/views/notifications/update_notification.py index d4f0a4c35..5294ba650 100644 --- a/funnel/views/notifications/update_notification.py +++ b/funnel/views/notifications/update_notification.py @@ -6,34 +6,54 @@ from baseframe import _, __ -from ...models import Account, NewUpdateNotification, Update +from ...models import Account, Project, ProjectUpdateNotification, Update from ...transports.sms import SmsPriority, SmsTemplate from ..helpers import shortlink from ..notification import RenderNotification from .mixins import TemplateVarMixin -class UpdateTemplate(TemplateVarMixin, SmsTemplate): - """DLT registered template for Updates.""" +class UpdateMergedTitleTemplate(TemplateVarMixin, SmsTemplate): + """DLT registered template for updates when the project has a merged title.""" registered_template = ( 'There is an update in {#var#}: {#var#}\n\nhttps://bye.li to stop -Hasgeek' ) template = ( - "There is an update in {account}: {url}\n\nhttps://bye.li to stop -Hasgeek" + "There is an update in {project}: {url}\n\nhttps://bye.li to stop -Hasgeek" ) - plaintext_template = "There is an update in {account}: {url}" + plaintext_template = "There is an update in {project}: {url}" message_priority = SmsPriority.NORMAL url: str -@NewUpdateNotification.renderer -class RenderNewUpdateNotification(RenderNotification): +class UpdateSplitTitleTemplate(TemplateVarMixin, SmsTemplate): + """DLT registered template for updates when the project has a split title.""" + + registered_template = ( + "There is an update in {#var#} / {#var#}. Details here: {#var#}" + "\n\nhttps://bye.li to stop -Hasgeek" + ) + template = ( + "There is an update in {account_title} / {project_title}. Details here: {url}" + "\n\nhttps://bye.li to stop -Hasgeek" + ) + plaintext_template = ( + "There is an update in {account_title} / {project_title}: {url}" + ) + message_priority = SmsPriority.NORMAL + + url: str + + +@ProjectUpdateNotification.renderer +class RenderProjectUpdateNotification(RenderNotification): """Notify crew and participants when the project has a new update.""" + project: Project update: Update - aliases = {'document': 'update'} + aliases = {'document': 'project', 'fragment': 'update'} emoji_prefix = "📰 " reason = __( "You are receiving this because you have registered for this or related" @@ -52,7 +72,9 @@ def actor(self) -> Account: return self.update.created_by def web(self) -> str: - return render_template('notifications/update_new_web.html.jinja2', view=self) + return render_template( + 'notifications/project_update_web.html.jinja2', view=self + ) def email_subject(self) -> str: return self.emoji_prefix + _("{update} ({project})").format( @@ -60,11 +82,22 @@ def email_subject(self) -> str: ) def email_content(self) -> str: - return render_template('notifications/update_new_email.html.jinja2', view=self) + return render_template( + 'notifications/project_update_email.html.jinja2', view=self + ) - def sms(self) -> UpdateTemplate: - return UpdateTemplate( - account=self.update.project.account, + def sms(self) -> UpdateMergedTitleTemplate | UpdateSplitTitleTemplate: + if len(self.update.project.title_parts) == 1: + return UpdateMergedTitleTemplate( + project=self.update.project, + url=shortlink( + self.update.url_for(_external=True, **self.tracking_tags('sms')), + shorter=True, + ), + ) + return UpdateSplitTitleTemplate( + project_title=self.update.project, + account_title=self.update.project.account, url=shortlink( self.update.url_for(_external=True, **self.tracking_tags('sms')), shorter=True, diff --git a/funnel/views/update.py b/funnel/views/update.py index d11068138..390cf1589 100644 --- a/funnel/views/update.py +++ b/funnel/views/update.py @@ -19,7 +19,7 @@ from .. import app from ..auth import current_auth from ..forms import SavedProjectForm, UpdateForm, UpdatePinForm -from ..models import Account, NewUpdateNotification, Project, Update, db +from ..models import Account, Project, ProjectUpdateNotification, Update, db from ..typing import ReturnRenderWith, ReturnView from .helpers import html_in_json, render_redirect from .login_session import requires_login, requires_sudo @@ -30,7 +30,7 @@ @Project.views('updates') @route('///updates', init_app=app) -class ProjectUpdatesView(ProjectViewBase): +class ProjectUpdateView(ProjectViewBase): @route('', methods=['GET']) @render_with(html_in_json('project_updates.html.jinja2')) @requires_roles({'reader'}) @@ -128,7 +128,11 @@ def publish(self) -> ReturnView: db.session.commit() flash(_("The update has been published"), 'success') if first_publishing: - dispatch_notification(NewUpdateNotification(document=self.obj)) + dispatch_notification( + ProjectUpdateNotification( + document=self.obj.project, fragment=self.obj + ) + ) else: flash( _("There was an error publishing this update. Reload and try again"), @@ -143,8 +147,11 @@ def edit(self) -> ReturnView: form = UpdatePinForm(obj=self.obj) else: form = UpdateForm(obj=self.obj) + if self.obj.state.PUBLISHED: + # Don't allow visibility change in a published update + del form.visibility if form.validate_on_submit(): - form.populate_obj(self.obj) + form.populate_obj(self.obj.current_access()) db.session.commit() if request.form.get('form.id') == 'pin': return {'status': 'ok', 'is_pinned': self.obj.is_pinned} diff --git a/migrations/versions/0afd20b31eb6_update_visibility_state_change.py b/migrations/versions/0afd20b31eb6_update_visibility_state_change.py new file mode 100644 index 000000000..02d01eb82 --- /dev/null +++ b/migrations/versions/0afd20b31eb6_update_visibility_state_change.py @@ -0,0 +1,50 @@ +"""Update visibility state change. + +Revision ID: 0afd20b31eb6 +Revises: 16c4e4bc3fe0 +Create Date: 2024-03-12 14:36:01.196118 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = '0afd20b31eb6' +down_revision: str = '16c4e4bc3fe0' +branch_labels: str | tuple[str, ...] | None = None +depends_on: str | tuple[str, ...] | None = None + +update = sa.table('update', sa.column('visibility_state')) + + +def upgrade(engine_name: str = '') -> None: + """Upgrade all databases.""" + # Do not modify. Edit `upgrade_` instead + globals().get(f'upgrade_{engine_name}', lambda: None)() + + +def downgrade(engine_name: str = '') -> None: + """Downgrade all databases.""" + # Do not modify. Edit `downgrade_` instead + globals().get(f'downgrade_{engine_name}', lambda: None)() + + +def upgrade_() -> None: + """Upgrade default database.""" + op.drop_constraint('update_visibility_state_check', 'update', type_='check') + op.create_check_constraint( + 'update_visibility_state_check', + 'update', + update.c.visibility_state.in_([1, 2, 3]), + ) + + +def downgrade_() -> None: + """Downgrade default database.""" + op.drop_constraint('update_visibility_state_check', 'update', type_='check') + op.create_check_constraint( + 'update_visibility_state_check', + 'update', + update.c.visibility_state.in_([1, 2]), + ) diff --git a/migrations/versions/bd377f7c3b3b_update_notification_data.py b/migrations/versions/bd377f7c3b3b_update_notification_data.py new file mode 100644 index 000000000..768ca9ec1 --- /dev/null +++ b/migrations/versions/bd377f7c3b3b_update_notification_data.py @@ -0,0 +1,95 @@ +"""Update notification data. + +Revision ID: bd377f7c3b3b +Revises: 0afd20b31eb6 +Create Date: 2024-03-22 18:28:50.178325 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = 'bd377f7c3b3b' +down_revision: str = '0afd20b31eb6' +branch_labels: str | tuple[str, ...] | None = None +depends_on: str | tuple[str, ...] | None = None + + +notification = sa.table( + 'notification', + sa.column('type', sa.Unicode()), + sa.column('document_uuid', sa.Uuid()), + sa.column('fragment_uuid', sa.Uuid()), +) +project = sa.table( + 'project', + sa.column('id', sa.Integer()), + sa.column('uuid', sa.Uuid()), +) +update = sa.table( + 'update', + sa.column('id', sa.Integer()), + sa.column('uuid', sa.Uuid()), + sa.column('project_id', sa.Integer()), +) +notification_preferences = sa.table( + 'notification_preferences', + sa.column('notification_type'), +) + + +def upgrade(engine_name: str = '') -> None: + """Upgrade all databases.""" + # Do not modify. Edit `upgrade_` instead + globals().get(f'upgrade_{engine_name}', lambda: None)() + + +def downgrade(engine_name: str = '') -> None: + """Downgrade all databases.""" + # Do not modify. Edit `downgrade_` instead + globals().get(f'downgrade_{engine_name}', lambda: None)() + + +def upgrade_() -> None: + """Upgrade default database.""" + # Rename type for update notification, move update UUID from document to fragment, + # and insert project UUID as document + op.execute( + notification.update() + .values( + type='project_update', + document_uuid=project.c.uuid, + fragment_uuid=update.c.uuid, + ) + .where( + notification.c.type == 'update_new', + notification.c.document_uuid == update.c.uuid, + update.c.project_id == project.c.id, + ) + ) + op.execute( + notification_preferences.update() + .values(notification_type='project_update') + .where(notification_preferences.c.notification_type == 'update_new') + ) + + +def downgrade_() -> None: + """Downgrade default database.""" + # Restore old notification type name, move update UUID from fragment to document, + # and set fragment to None + op.execute( + notification_preferences.update() + .values(notification_type='update_new') + .where(notification_preferences.c.notification_type == 'project_update') + ) + op.execute( + notification.update() + .values( + type='update_new', + document_uuid=notification.c.fragment_uuid, + fragment_uuid=None, + ) + .where(notification.c.type == 'project_update') + ) diff --git a/sample.env b/sample.env index 829c62f31..3df552bdd 100644 --- a/sample.env +++ b/sample.env @@ -241,7 +241,8 @@ FLASK_SMS_DLT_TEMPLATE_IDS__registration_confirmation_template=null FLASK_SMS_DLT_TEMPLATE_IDS__registration_confirmation_with_next_template=null FLASK_SMS_DLT_TEMPLATE_IDS__proposal_received_template=null FLASK_SMS_DLT_TEMPLATE_IDS__proposal_submitted_template=null -FLASK_SMS_DLT_TEMPLATE_IDS__update_template=null +FLASK_SMS_DLT_TEMPLATE_IDS__update_merged_title_template=null +FLASK_SMS_DLT_TEMPLATE_IDS__update_split_title_template=null FLASK_SMS_DLT_TEMPLATE_IDS__comment_project_template=null FLASK_SMS_DLT_TEMPLATE_IDS__comment_proposal_template=null FLASK_SMS_DLT_TEMPLATE_IDS__comment_reply_template=null diff --git a/tests/conftest.py b/tests/conftest.py index 8989ac1f7..6aebe3e3d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1267,9 +1267,7 @@ def logout() -> None: # TODO: Test this client.delete_cookie('lastuser', domain=app.config['LASTUSER_COOKIE_DOMAIN']) - return SimpleNamespace( # pyright: ignore[reportGeneralTypeIssues] - as_=as_, logout=logout - ) + return SimpleNamespace(as_=as_, logout=logout) # pyright: ignore[reportReturnType] # --- Sample data: users, organizations, projects, etc --------------------------------- diff --git a/tests/features/update/update_members.feature b/tests/features/update/update_members.feature new file mode 100644 index 000000000..e69de29bb diff --git a/tests/features/update/update_participants.feature b/tests/features/update/update_participants.feature new file mode 100644 index 000000000..6fb572fab --- /dev/null +++ b/tests/features/update/update_participants.feature @@ -0,0 +1,4 @@ +Feature: Post an update for participants of a project. + Havelock Vetinari is the editor of Ankh Morpork's annual expo. + Vetinari wants to share updates about the 2011 edition only with the participants of the 2011 edition. + diff --git a/tests/features/update/update_pinned.feature b/tests/features/update/update_pinned.feature new file mode 100644 index 000000000..e69de29bb diff --git a/tests/features/update/update_public.feature b/tests/features/update/update_public.feature new file mode 100644 index 000000000..7905f983f --- /dev/null +++ b/tests/features/update/update_public.feature @@ -0,0 +1,57 @@ +Feature: Post a public update + Havelock Vetinari is the editor of Ankh Morpork's annual expo. + Vetinari wants to announce the 2011 edition, making the announcement publicly + available and sending a copy to everyone who attended previous editions. + + Scenario: Vetinari posts a public notice + Given the 2011 expo project has been published + And Vetinari has drafted an update in this project for the public announcement + When Vetinari publishes the update + Then the update is sent to everyone who signed up for the 2011 edition + And the update is sent to everyone who signed up for a past edition + + Scenario: Vetinari posts a public notice + Given the 2011 expo project has been published + And Vimes is also an editor of the 2011 expo project + And Vimes has drafted an update in this project for the public announcement + And Vimes has not published the update, leaving it to Vetinari to review + When Vetinari publishes the update + Then the update is sent to everyone who signed up for the 2011 edition + And the update is sent to everyone who signed up for a past edition + + Scenario: Vimes posts a public notice about the venue and facilities. + And Vimes is also an editor of the 2011 expo project + Given that Vimes has drafted an update about the venue, parking and check-in procedure + And Vimes has published the update + Then the update is sent to everyone who signed up for the 2011 edition + And the update is sent to everyone who signed up for a past edition + + Scenario: Vimes posts a public notice about the speakers of 2011 expo. + Given that Vimes has drafted an update in this project for the public announcement + And Vimes has added links for each speaker's talk. + Then the update is sent to everyone who signed up for the 2011 edition + And the update is sent to everyone who signed up for a past edition + And Twoflower is redirected to the descriptions each time he clicks on the talk links + + Scenario: Vimes posts a public notice about the schedule for 2011 expo. + Given that Vimes has drafted an update in this project for the public announcement + And Vimes has added the schedule in a tabular format in markdown + Then the update is sent to everyone who signed up for the 2011 edition + And the update is sent to everyone who signed up for a past edition + And Twoflower can the see the schedule in a tabular format in the update + + Scenario: Vimes posts a public notice. + And Vimes has drafted weekly reminders, leading to 2011 expo + And Vimes has scheduled the reminder updates to go out weekly on Friday at 10 AM + Given that Vimes schedules the weekly updates + Then the weekly update is sent to everyone who signed up for the 2011 edition on the schedule day and time + And the weekly update is sent to everyone who signed up for a past edition on the schedule day and time + + Scenario: Twoflower needs a reminder about 2011 expo one week before. + Given that Twoflower has registered to attend 2011 expo + Then Twoflower's RSVP should show up on his device calendar with the option to be reminded about 2011 expo one week before + + Scenario: Twoflower has registered to attend 2011 expo but is unable to find the venue and directions. + And Twoflower searches for the venue and schedule notification + Given that Twoflower has registered to attend 2011 expo + Then Twoflower can search updates to find venue and directions update diff --git a/tests/unit/models/account_membership_test.py b/tests/unit/models/account_membership_test.py new file mode 100644 index 000000000..887bac871 --- /dev/null +++ b/tests/unit/models/account_membership_test.py @@ -0,0 +1,37 @@ +"""Tests for AccountMembership models.""" + +import pytest + +from funnel import models + +from ...conftest import scoped_session + + +@pytest.fixture() +def placeholder_account(db_session: scoped_session) -> models.Placeholder: + obj = models.Placeholder(name='placeholder', title='Placeholder') + db_session.add(obj) + return obj + + +@pytest.mark.parametrize( + 'account_fixture', + [ + 'user_vetinari', + 'user_rincewind', + 'user_twoflower', + 'org_ankhmorpork', + 'org_uu', + 'placeholder_account', + ], +) +@pytest.mark.parametrize('role', ['follower', 'member', 'admin', 'owner']) +def test_user_account_has_roles_on_self( + request: pytest.FixtureRequest, account_fixture: str, role: str +) -> None: + """User accounts grant roles to self, but other account types don't.""" + account: models.Account = request.getfixturevalue(account_fixture) + if account.is_user_profile: + assert role in account.roles_for(account) + else: + assert role not in account.roles_for(account) diff --git a/tests/unit/models/merge_notification_test.py b/tests/unit/models/merge_notification_test.py index 995a4cded..e397393c7 100644 --- a/tests/unit/models/merge_notification_test.py +++ b/tests/unit/models/merge_notification_test.py @@ -75,7 +75,7 @@ def notification_recipient1( eventid=notification.eventid, recipient_id=fixtures.user1.id, notification_id=notification.id, - role=models.OrganizationAdminMembershipNotification.roles[-1], + role=notification.dispatch_roles[-1], ) db_session.add(nr) db_session.commit() @@ -92,7 +92,7 @@ def notification_recipient2( eventid=notification.eventid, recipient_id=fixtures.user2.id, notification_id=notification.id, - role=models.OrganizationAdminMembershipNotification.roles[-1], + role=notification.dispatch_roles[-1], ) db_session.add(nr) db_session.commit() diff --git a/tests/unit/models/models_utils_test.py b/tests/unit/models/models_utils_test.py index 800b333b1..7672360af 100644 --- a/tests/unit/models/models_utils_test.py +++ b/tests/unit/models/models_utils_test.py @@ -194,7 +194,7 @@ def test_getuser_anchor( ) assert models.getuser('mort@example.net', True) == (user_mort, user_mort.email) - # Retrival by email claim only works when there is no verified email address + # Retrieval by email claim only works when there is no verified email address assert models.getuser('twoflower@example.org', True) != ( user_wolfgang, user_wolfgang.emailclaims[0], diff --git a/tests/unit/models/notification_test.py b/tests/unit/models/notification_test.py index a49a5c44f..482c782fa 100644 --- a/tests/unit/models/notification_test.py +++ b/tests/unit/models/notification_test.py @@ -1,6 +1,7 @@ """Tests for Notification and UserNotification models.""" # pylint: disable=possibly-unused-variable,redefined-outer-name +# pyright: reportAssignmentType=false from __future__ import annotations @@ -36,7 +37,7 @@ class TestNewUpdateNotification( category = models.notification_categories.participant description = "When a project posts an update" - roles = ['project_crew', 'project_participant'] + dispatch_roles = ['project_crew', 'project_participant'] class TestEditedUpdateNotification( ProjectIsParent, @@ -46,7 +47,7 @@ class TestEditedUpdateNotification( ): """Notifications of edited updates (test edition).""" - roles = ['project_crew', 'project_participant'] + dispatch_roles = ['project_crew', 'project_participant'] class TestProposalReceivedNotification( ProjectIsParent, @@ -57,7 +58,7 @@ class TestProposalReceivedNotification( category = models.notification_categories.project_crew description = "When my project receives a new proposal" - roles = ['project_editor'] + dispatch_roles = ['project_editor'] sa_orm.configure_mappers() return SimpleNamespace(**locals()) @@ -70,7 +71,8 @@ def project_fixtures( """Provide users, one org and one project, for tests on them.""" user_owner = models.User(username='user_owner', fullname="User Owner") user_owner.add_email('owner@example.com') - + user_admin = models.User(username='user_admin', fullname="User Admin") + user_admin.add_email('admin@example.com') user_editor = models.User(username='user_editor', fullname="User Editor") user_editor.add_email('editor@example.com') user_editor_phone = models.AccountPhone(account=user_editor, phone='+12345678900') @@ -89,10 +91,17 @@ def project_fixtures( org = models.Organization( name='notifications_org', title="Organization", owner=user_owner ) - + admin_membership = models.AccountMembership( + account=org, + member=user_admin, + is_owner=False, + granted_by=user_owner, + ) db_session.add_all( [ + admin_membership, user_owner, + user_admin, user_editor, user_editor_phone, user_participant, @@ -196,6 +205,10 @@ def test_update_roles(project_fixtures: SimpleNamespace, update: models.Update) assert 'project_editor' in owner_roles assert 'project_crew' in owner_roles assert 'project_participant' in owner_roles + assert 'account_member' in owner_roles + + admin_roles = update.roles_for(project_fixtures.user_admin) + assert 'account_member' in admin_roles editor_roles = update.roles_for(project_fixtures.user_editor) assert 'project_editor' in editor_roles @@ -226,16 +239,18 @@ def test_update_notification_structure( update: models.Update, db_session: scoped_session, ) -> None: - """Test whether a NewUpdateNotification has the appropriate structure.""" + """Test whether a TestNewUpdateNotification has the appropriate structure.""" project_fixtures.refresh() - notification = notification_types.TestNewUpdateNotification(update) + notification: models.Notification = notification_types.TestNewUpdateNotification( + update + ) db_session.add(notification) db_session.commit() - assert notification.type == 'update_new_test' + assert notification.type_ == 'update_new_test' assert notification.document == update assert notification.fragment is None - assert notification.roles == ['project_crew', 'project_participant'] + assert notification.dispatch_roles == ['project_crew', 'project_participant'] assert notification.preference_context == project_fixtures.org load_notification = models.Notification.query.first() @@ -253,7 +268,8 @@ def test_update_notification_structure( # A second call to dispatch() will yield nothing assert not list(notification.dispatch()) - # Notifications are issued strictly in the order specified in cls.roles + # Notifications are issued strictly in the order specified in + # notification.dispatch_roles role_order: list[str] = [] for nr in notification_recipients: if nr.role in role_order: @@ -264,7 +280,7 @@ def test_update_notification_structure( assert role_order == ['project_crew', 'project_participant'] # Notifications are correctly assigned by priority of role - role_users: dict[str, set[models.User]] = {} + role_users: dict[str, set[models.Account]] = {} for nr in notification_recipients: role_users.setdefault(nr.role, set()).add(nr.recipient) diff --git a/tests/unit/models/update_test.py b/tests/unit/models/update_test.py new file mode 100644 index 000000000..de9359c87 --- /dev/null +++ b/tests/unit/models/update_test.py @@ -0,0 +1,233 @@ +"""Tests for the Update model.""" + +# pylint: disable=redefined-outer-name + +from itertools import permutations + +import pytest + +from funnel import models + +from ...conftest import scoped_session + +# --- Fixtures ------------------------------------------------------------------------- + + +@pytest.fixture() +def public_update( + db_session: scoped_session, + project_expo2010: models.Project, + user_vetinari: models.User, +) -> models.Update: + """Public update fixture.""" + update = models.Update( + project=project_expo2010, + created_by=user_vetinari, + visibility='public', + title="Public update", + body="Public update body", + ) + db_session.add(update) + return update + + +@pytest.fixture() +def participant_update( + db_session: scoped_session, + project_expo2010: models.Project, + user_vetinari: models.User, +) -> models.Update: + """Participants-only update fixture.""" + update = models.Update( + project=project_expo2010, + created_by=user_vetinari, + visibility='participants', + title="Participant update", + body="Participant update body", + ) + db_session.add(update) + return update + + +@pytest.fixture() +def member_update( + db_session: scoped_session, + project_expo2010: models.Project, + user_vetinari: models.User, +) -> models.Update: + """Members-only update fixture.""" + update = models.Update( + project=project_expo2010, + created_by=user_vetinari, + visibility='members', + title="Member update", + body="Member update body", + ) + db_session.add(update) + return update + + +@pytest.fixture() +def vimes_admin( + db_session: scoped_session, + org_ankhmorpork: models.Organization, + user_vetinari: models.User, + user_vimes: models.User, +) -> models.AccountMembership: + """Org admin membership for user Vimes.""" + membership = models.AccountMembership( + account=org_ankhmorpork, + member=user_vimes, + is_owner=False, + granted_by=user_vetinari, + ) + db_session.add(membership) + return membership + + +@pytest.fixture() +def ridcully_editor( + db_session: scoped_session, + user_vetinari: models.User, + user_ridcully: models.User, + project_expo2010: models.Project, +) -> models.ProjectMembership: + """Project editor membership for Ridcully.""" + membership = models.ProjectMembership( + project=project_expo2010, + member=user_ridcully, + is_editor=True, + is_promoter=False, + is_usher=False, + granted_by=user_vetinari, + ) + db_session.add(membership) + return membership + + +@pytest.fixture() +def rincewind_participant( + db_session: scoped_session, + project_expo2010: models.Project, + user_rincewind: models.User, +) -> models.Rsvp: + rsvp = models.Rsvp(project=project_expo2010, participant=user_rincewind) + db_session.add(rsvp) + rsvp.rsvp_yes() + return rsvp + + +# --- Tests ---------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ('update1', 'update2', 'update3'), + permutations(['public_update', 'participant_update', 'member_update']), +) +@pytest.mark.parametrize('delete', [True, False]) +def test_update_numbering( + request: pytest.FixtureRequest, + db_session: scoped_session, + user_vetinari: models.User, + update1: str, + update2: str, + update3: str, + delete: bool, +) -> None: + """Update numbers are incremental even if a previous update was deleted.""" + for number, fixture_name in enumerate([update1, update2, update3], 1): + obj: models.Update = request.getfixturevalue(fixture_name) + obj.publish(user_vetinari) + db_session.commit() + assert obj.number == number + if delete: + obj.delete(user_vetinari) + db_session.commit() + + +@pytest.mark.parametrize( + 'update_fixture', ['public_update', 'participant_update', 'member_update'] +) +@pytest.mark.usefixtures('vimes_admin', 'ridcully_editor', 'rincewind_participant') +def test_draft_update_is_not_accessible( + request: pytest.FixtureRequest, + user_vetinari: models.User, + user_ridcully: models.User, + user_vimes: models.User, + user_rincewind: models.User, + user_twoflower: models.User, + update_fixture: str, +) -> None: + """A draft or deleted update is not accessible to anyone except project crew.""" + update: models.Update = request.getfixturevalue(update_fixture) + assert update.state.DRAFT + assert not update.state.PUBLISHED + # The project editor gets 'reader' role courtesy of being a crew member + assert 'reader' in update.roles_for(user_vetinari) + assert 'reader' in update.roles_for(user_ridcully) + # Any other user does not as the update is still a draft + assert 'reader' not in update.roles_for(user_vimes) + assert 'reader' not in update.roles_for(user_rincewind) + assert 'reader' not in update.roles_for(user_twoflower) + assert 'reader' not in update.roles_for(None) + + +@pytest.mark.usefixtures('vimes_admin', 'ridcully_editor', 'rincewind_participant') +def test_public_update_grants_reader_role_to_all( + db_session: scoped_session, + user_vetinari: models.User, + user_vimes: models.User, + user_ridcully: models.User, + user_rincewind: models.User, + user_twoflower: models.User, + public_update: models.Update, +) -> None: + """A public update grants 'reader' role to all after it is published.""" + public_update.publish(user_vetinari) + db_session.commit() + assert public_update.state.PUBLISHED + # Reader role is granted to all users (with or without specific roles; even anon) + assert 'reader' in public_update.roles_for(user_twoflower) + assert 'reader' in public_update.roles_for(user_vimes) + assert 'reader' in public_update.roles_for(user_ridcully) + assert 'reader' in public_update.roles_for(user_rincewind) + assert 'reader' in public_update.roles_for(user_twoflower) + assert 'reader' in public_update.roles_for(None) + + +@pytest.mark.usefixtures('vimes_admin', 'ridcully_editor', 'rincewind_participant') +def test_participant_update_grants_reader_role_to_participants( + db_session: scoped_session, + user_vetinari: models.User, + user_ridcully: models.User, + user_vimes: models.User, + user_rincewind: models.User, + user_twoflower: models.User, + participant_update: models.Update, +) -> None: + """A participant update grants 'reader' role to participants only.""" + participant_update.publish(user_vetinari) + db_session.commit() + # Reader role is granted to participants (and crew) but not anyone else + assert 'reader' in participant_update.roles_for(user_vetinari) # Crew + assert 'reader' in participant_update.roles_for(user_ridcully) # Crew + assert 'reader' in participant_update.roles_for(user_rincewind) # Participant + assert 'reader' not in participant_update.roles_for(user_vimes) # Admin/member + assert 'reader' not in participant_update.roles_for(user_twoflower) # Unrelated + assert 'reader' not in participant_update.roles_for(None) # Anonymous + + +@pytest.mark.usefixtures('vimes_admin', 'ridcully_editor', 'rincewind_participant') +def test_member_update_grants_reader_role_to_members_only( + db_session: scoped_session, + user_vetinari: models.User, + user_vimes: models.User, + member_update: models.Update, +) -> None: + """A member update grants 'reader' role to project members only.""" + member_update.publish(user_vetinari) + db_session.commit() + assert set(member_update.actors_with({'account_member'})) == { + user_vetinari, + user_vimes, + } diff --git a/tests/unit/views/notification_test.py b/tests/unit/views/notification_test.py index 5c937fbf9..56e36bf39 100644 --- a/tests/unit/views/notification_test.py +++ b/tests/unit/views/notification_test.py @@ -74,7 +74,9 @@ def update_notification_recipient( project_update: models.Update, ) -> models.NotificationRecipient: """Get a user notification for the update fixture.""" - notification = models.NewUpdateNotification(project_update) + notification = models.ProjectUpdateNotification( + document=project_update.project, fragment=project_update + ) db_session.add(notification) db_session.commit() @@ -166,21 +168,28 @@ def test_template_var_mixin() -> None: t1.var_max_length = 40 p1 = SimpleNamespace( - title='Ankh-Morpork 2010', joined_title='Ankh-Morpork / Ankh-Morpork 2010' + title='Ankh-Morpork 2010', joined_title='Discworld / Ankh-Morpork 2010' ) u1 = SimpleNamespace( pickername='Havelock Vetinari (@vetinari)', title='Havelock Vetinari' ) u2 = SimpleNamespace(pickername='Twoflower', title='Twoflower') + a1 = SimpleNamespace(pickername='Discworld (@discworld)', title='Discworld') t1.project = cast(models.Project, p1) + t1.project_title = cast(models.Project, p1) t1.user = cast(models.User, u2) t1.actor = cast(models.User, u1) + t1.account = cast(models.Account, a1) + t1.account_title = cast(models.Account, a1) assert isinstance(t1.project, str) assert isinstance(t1.actor, str) assert isinstance(t1.user, str) - assert t1.project == 'Ankh-Morpork / Ankh-Morpork 2010' + assert t1.project == 'Discworld / Ankh-Morpork 2010' + assert t1.project_title == 'Ankh-Morpork 2010' assert t1.actor == 'Havelock Vetinari (@vetinari)' assert t1.user == 'Twoflower' + assert t1.account == 'Discworld (@discworld)' + assert t1.account_title == 'Discworld' # Do this again to confirm truncation at a smaller size t1.var_max_length = 20