From 7705356a7c009924acf6903d9558f867b457b904 Mon Sep 17 00:00:00 2001 From: Ocean Dekeyser Date: Thu, 22 Oct 2026 15:42:38 +0200 Subject: [PATCH] Chat Guests, "Only my chats" and Students with several chats (#93) The Product Owner, a Teacher or the Superuser adds any active User to a Project's chat as a Guest (Members can't); Guests read the whole history, write like the team, can leave, and can be removed. Each chat shows its people (Product Owner, Members, Guests). The Chats screen has "Only my chats" (on by default); a Teacher's badge also counts chats where they are a Guest; a Student in several chats gets a list. Archiving ends Guest access. Migration 0015. Co-Authored-By: Claude Opus 5.5 --- CONTEXT.md | 6 +- .../migrations/versions/0015_chat_guests.py | 42 +++++ backend/src/pmp_backend/api/chat.py | 75 +++++++- backend/src/pmp_backend/models.py | 19 ++ backend/src/pmp_backend/services/chat.py | 164 ++++++++++++++--- backend/tests/test_chat.py | 8 +- backend/tests/test_chat_guests.py | 166 ++++++++++++++++++ docs/CHANGELOG.md | 1 + frontend/src/pmp_frontend/api.py | 41 ++++- frontend/src/pmp_frontend/chat_page.py | 164 +++++++++++++++-- frontend/tests/test_chat_guests.py | 106 +++++++++++ 11 files changed, 745 insertions(+), 47 deletions(-) create mode 100644 backend/migrations/versions/0015_chat_guests.py create mode 100644 backend/tests/test_chat_guests.py create mode 100644 frontend/tests/test_chat_guests.py diff --git a/CONTEXT.md b/CONTEXT.md index e42202f..a833ab8 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -117,9 +117,13 @@ A Project's one group chat (spec #91): short messages of its Chat people, shown _Avoid_: Forum, channel, conversation, group **Chat people**: -Whom a Project's Chat is for: its Members and its Product Owner. Only they count for the Unread badge; a Member who is removed or moved loses access to the Chat at once, and a new Member sees its whole history. +Whom a Project's Chat is for: its Members, its Product Owner and its Guests. Only they count for the Unread badge; a Member who is removed or moved loses access to the Chat at once, and a new Member sees its whole history. _Avoid_: Participants, chat members +**Guest**: +Any active User added to a Project's Chat on top of its team, by the Product Owner, a Teacher or the Superuser (never by Members). A Guest reads the whole history and writes like the team, can leave, and can be removed by the same staff; archiving the Project ends their access. +_Avoid_: Visitor, external member, invitee + **Unread**: The messages of others in a Chat after the last one a User has seen; opening the Chat reads them. The Chat item in the navigation shows how many there are in the User's own Chats (for a Teacher: those where they are one of the Chat people). _Avoid_: New messages, notifications diff --git a/backend/migrations/versions/0015_chat_guests.py b/backend/migrations/versions/0015_chat_guests.py new file mode 100644 index 0000000..bfa8ad2 --- /dev/null +++ b/backend/migrations/versions/0015_chat_guests.py @@ -0,0 +1,42 @@ +"""chat guests + +Revision ID: 0015 +Revises: 0014 +Create Date: 2026-10-22 15:25:00 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "0015" +down_revision: str | Sequence[str] | None = "0014" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Upgrade schema.""" + # Guests in a Project's Chat (spec #91), on top of its Chat people. + op.create_table( + "chat_guests", + sa.Column("project_id", sa.Integer(), nullable=False), + sa.Column("user_id", sa.Integer(), nullable=False), + sa.Column( + "added_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.ForeignKeyConstraint(["project_id"], ["projects.id"], ondelete="CASCADE"), + sa.ForeignKeyConstraint(["user_id"], ["users.id"], ondelete="CASCADE"), + sa.PrimaryKeyConstraint("project_id", "user_id"), + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_table("chat_guests") diff --git a/backend/src/pmp_backend/api/chat.py b/backend/src/pmp_backend/api/chat.py index d92709c..5114422 100644 --- a/backend/src/pmp_backend/api/chat.py +++ b/backend/src/pmp_backend/api/chat.py @@ -18,6 +18,19 @@ class ChatOut(BaseModel): title: str # Archived Projects' Chats are read-only. read_only: bool + # Teachers and the Superuser add and remove Guests. + can_manage_guests: bool + # The User is a Guest here, so they may leave. + is_guest: bool + + +class ChatPersonOut(PersonOut): + # "Member", "Product Owner" or "Guest". + kind: str + + +class GuestIn(BaseModel): + user_id: int class MessageIn(BaseModel): @@ -69,13 +82,57 @@ class UnreadOut(BaseModel): @router.get("/projects/{project_id}/chat") def get_chat(project_id: int, session: SessionDep, user: CurrentUserDep) -> ChatOut: project = chat.readable_project(session, user, project_id) + read_only = project.status == ProjectStatus.ARCHIVED.value return ChatOut( project_id=project.id, title=project.title, - read_only=project.status == ProjectStatus.ARCHIVED.value, + read_only=read_only, + can_manage_guests=chat.is_staff(user) and not read_only, + is_guest=chat.is_guest(session, user, project) and not read_only, ) +@router.get("/projects/{project_id}/chat/people") +def chat_people( + project_id: int, session: SessionDep, user: CurrentUserDep +) -> list[ChatPersonOut]: + """The Product Owner, the Members, then the Guests.""" + return [ + ChatPersonOut(**PersonOut.of(person.user).model_dump(), kind=person.kind) + for person in chat.people(session, user, project_id) + ] + + +@router.get("/projects/{project_id}/chat/guests/candidates") +def guest_candidates( + project_id: int, session: SessionDep, user: CurrentUserDep +) -> list[PersonOut]: + """Active Users who could join as a Guest (Teachers and the Superuser).""" + return [ + PersonOut.of(candidate) + for candidate in chat.guest_candidates(session, user, project_id) + ] + + +@router.post("/projects/{project_id}/chat/guests", status_code=status.HTTP_201_CREATED) +def add_guest( + project_id: int, body: GuestIn, session: SessionDep, user: CurrentUserDep +) -> PersonOut: + return PersonOut.of(chat.add_guest(session, user, project_id, body.user_id)) + + +@router.delete( + "/projects/{project_id}/chat/guests/{user_id}", + status_code=status.HTTP_204_NO_CONTENT, +) +def remove_guest( + project_id: int, user_id: int, session: SessionDep, user: CurrentUserDep +) -> Response: + """Teachers and the Superuser remove a Guest; a Guest leaves with their own id.""" + chat.remove_guest(session, user, project_id, user_id) + return Response(status_code=status.HTTP_204_NO_CONTENT) + + @router.get("/projects/{project_id}/chat/messages") def list_messages( project_id: int, @@ -105,8 +162,20 @@ def mark_read( return Response(status_code=status.HTTP_204_NO_CONTENT) +@router.post( + "/projects/{project_id}/chat/leave", status_code=status.HTTP_204_NO_CONTENT +) +def leave_chat(project_id: int, session: SessionDep, user: CurrentUserDep) -> Response: + """A Guest leaves the Chat.""" + chat.remove_guest(session, user, project_id, user.id) + return Response(status_code=status.HTTP_204_NO_CONTENT) + + @router.get("/chats") -def list_chats(session: SessionDep, user: CurrentUserDep) -> list[ChatSummaryOut]: +def list_chats( + session: SessionDep, user: CurrentUserDep, mine: bool = False +) -> list[ChatSummaryOut]: + """With `mine`: only the Chats where the User is one of the Chat people.""" return [ ChatSummaryOut( project_id=summary.project_id, @@ -114,7 +183,7 @@ def list_chats(session: SessionDep, user: CurrentUserDep) -> list[ChatSummaryOut unread=summary.unread, mine=summary.mine, ) - for summary in chat.chats(session, user) + for summary in chat.chats(session, user, mine=mine) ] diff --git a/backend/src/pmp_backend/models.py b/backend/src/pmp_backend/models.py index 5e653bb..4b24259 100644 --- a/backend/src/pmp_backend/models.py +++ b/backend/src/pmp_backend/models.py @@ -352,3 +352,22 @@ class ChatRead(Base): ForeignKey("users.id", ondelete="CASCADE"), primary_key=True ) last_read_id: Mapped[int] = mapped_column(default=0) + + +class ChatGuest(Base): + """A Guest in a Project's Chat: any User added by the Product Owner, a + Teacher or the Superuser, on top of the Members and the Product Owner.""" + + __tablename__ = "chat_guests" + + project_id: Mapped[int] = mapped_column( + ForeignKey("projects.id", ondelete="CASCADE"), primary_key=True + ) + user_id: Mapped[int] = mapped_column( + ForeignKey("users.id", ondelete="CASCADE"), primary_key=True + ) + added_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), server_default=func.now() + ) + + user: Mapped[User] = relationship(lazy="joined") diff --git a/backend/src/pmp_backend/services/chat.py b/backend/src/pmp_backend/services/chat.py index 6bfcdfb..350db4d 100644 --- a/backend/src/pmp_backend/services/chat.py +++ b/backend/src/pmp_backend/services/chat.py @@ -1,8 +1,8 @@ """The Project Chat (spec #91): one group chat per Project. -Its Chat people are the Members and the Product Owner; every Teacher and the -Superuser can also read and write in every Chat. Unread counts come from how far -each User read a Chat (`ChatRead`). +Its Chat people are the Members, the Product Owner and the Guests; every +Teacher and the Superuser can also read and write in every Chat. Unread counts +come from how far each User read a Chat (`ChatRead`). """ from dataclasses import dataclass @@ -12,9 +12,10 @@ from sqlalchemy.orm import Session from pmp_backend.domain import ProjectStatus, Role -from pmp_backend.errors import Conflict, Forbidden, Refused -from pmp_backend.models import ChatMessage, ChatRead, Project, User +from pmp_backend.errors import Conflict, Forbidden, NotFound, Refused +from pmp_backend.models import ChatGuest, ChatMessage, ChatRead, Project, User from pmp_backend.services.projects import get_project +from pmp_backend.services.users import get_user MAX_LENGTH = 4000 # The newest messages a Chat opens with; older ones come later (search, #97). @@ -33,27 +34,57 @@ class ChatConflictError(ChatError, Conflict): pass +class ChatNotFoundError(ChatError, NotFound): + pass + + def is_staff(user: User) -> bool: return user.role in (Role.TEACHER, Role.SUPERUSER) -def is_chat_person(user: User, project: Project) -> bool: - """A Member or the Product Owner: whom the Chat is for.""" +def is_team(user: User, project: Project) -> bool: + """A Member or the Product Owner.""" return user.project_id == project.id or user.id == project.product_owner_id -def can_read(user: User, project: Project) -> bool: - return is_staff(user) or is_chat_person(user, project) +def guest_project_ids(session: Session, user: User) -> set[int]: + """The Projects in whose Chat the User is a Guest.""" + return set( + session.scalars( + select(ChatGuest.project_id).where(ChatGuest.user_id == user.id) + ) + ) + + +def is_guest(session: Session, user: User, project: Project) -> bool: + return session.get(ChatGuest, (project.id, user.id)) is not None + + +def is_chat_person(session: Session, user: User, project: Project) -> bool: + """A Member, the Product Owner or a Guest: whom the Chat is for. Guests lose + access when the Project is archived.""" + if is_team(user, project): + return True + active = project.status == ProjectStatus.ACTIVE.value + return active and is_guest(session, user, project) def readable_project(session: Session, user: User, project_id: int) -> Project: """The Project whose Chat `user` may read; refused otherwise.""" project = get_project(session, project_id) - if not can_read(user, project): + if not (is_staff(user) or is_chat_person(session, user, project)): raise ChatForbiddenError("You don't have access to this chat.") return project +def writable_project(session: Session, user: User, project_id: int) -> Project: + """The Project whose Chat `user` may write in: Archived ones are read-only.""" + project = readable_project(session, user, project_id) + if project.status == ProjectStatus.ARCHIVED.value: + raise ChatConflictError(f"{project.title} is archived: its chat is read-only.") + return project + + def messages( session: Session, user: User, project_id: int, after: int | None = None ) -> list[ChatMessage]: @@ -72,9 +103,7 @@ def messages( def send(session: Session, user: User, project_id: int, body: str) -> ChatMessage: - project = readable_project(session, user, project_id) - if project.status == ProjectStatus.ARCHIVED.value: - raise ChatConflictError(f"{project.title} is archived: its chat is read-only.") + project = writable_project(session, user, project_id) body = body.strip() if not body: raise ChatError("Write a message first.") @@ -121,13 +150,17 @@ class ChatSummary: mine: bool -def _visible_projects(session: Session, user: User) -> list[Project]: - """Staff see every Active Project's Chat; Students their own Project's.""" +def _visible_projects( + session: Session, user: User, guest_ids: set[int] +) -> list[Project]: + """Staff see every Active Project's Chat; Students their own Project's and + those where they are a Guest.""" query = select(Project).where(Project.status == ProjectStatus.ACTIVE.value) if not is_staff(user): - if user.project_id is None: + own = {user.project_id} if user.project_id is not None else set() + if not own | guest_ids: return [] - query = query.where(Project.id == user.project_id) + query = query.where(Project.id.in_(own | guest_ids)) return list(session.scalars(query.order_by(func.lower(Project.title)))) @@ -155,24 +188,107 @@ def _unread_counts(session: Session, user: User, project_ids: list[int]) -> dict return dict(rows.all()) -def chats(session: Session, user: User) -> list[ChatSummary]: - """The Chats the User sees, by title, with their Unread counts.""" - projects = _visible_projects(session, user) +def chats(session: Session, user: User, *, mine: bool = False) -> list[ChatSummary]: + """The Chats the User sees (with `mine`: only those where they are one of the + Chat people), by title, with their Unread counts.""" + guest_ids = guest_project_ids(session, user) + projects = _visible_projects(session, user, guest_ids) counts = _unread_counts(session, user, [project.id for project in projects]) - return [ + summaries = [ ChatSummary( project.id, project.title, counts.get(project.id, 0), - is_chat_person(user, project), + is_team(user, project) or project.id in guest_ids, ) for project in projects ] + return [summary for summary in summaries if summary.mine or not mine] def unread_total(session: Session, user: User) -> tuple[int, bool]: - """The badge: Unread messages in the User's own Chats (for Teachers, those of - their Projects as Product Owner), and whether they have any Chat at all.""" + """The badge: Unread messages in the User's own Chats (for Teachers, those + where they are the Product Owner or a Guest), and whether they have any Chat + at all.""" summaries = chats(session, user) mine = [summary for summary in summaries if summary.mine] return sum(summary.unread for summary in mine), bool(summaries) + + +# Who is in a Chat. + +MEMBER, PRODUCT_OWNER, GUEST = "Member", "Product Owner", "Guest" + + +@dataclass(frozen=True) +class ChatPerson: + user: User + # MEMBER, PRODUCT_OWNER or GUEST. + kind: str + + +def people(session: Session, user: User, project_id: int) -> list[ChatPerson]: + """The Chat people: the Product Owner, the Members, then the Guests.""" + project = readable_project(session, user, project_id) + found = [ChatPerson(project.product_owner, PRODUCT_OWNER)] + found += [ChatPerson(member, MEMBER) for member in project.members] + if project.status == ProjectStatus.ACTIVE.value: + found += [ChatPerson(guest.user, GUEST) for guest in _guests(session, project)] + return found + + +def _guests(session: Session, project: Project) -> list[ChatGuest]: + return list( + session.scalars( + select(ChatGuest) + .join(ChatGuest.user) + .where(ChatGuest.project_id == project.id) + .order_by(User.last_name, User.first_name) + ) + ) + + +def _managed_project(session: Session, actor: User, project_id: int) -> Project: + """The Project whose Guests `actor` may change: staff only, Active only.""" + if not is_staff(actor): + raise ChatForbiddenError( + "Only the Product Owner, Teachers and the Superuser add or remove Guests." + ) + return writable_project(session, actor, project_id) + + +def guest_candidates(session: Session, actor: User, project_id: int) -> list[User]: + """Active Users who could be added as a Guest: not yet one of the Chat people.""" + project = _managed_project(session, actor, project_id) + inside = {person.user.id for person in people(session, actor, project.id)} + users = session.scalars( + select(User) + .where(User.is_active.is_(True)) + .order_by(User.last_name, User.first_name) + ) + return [user for user in users if user.id not in inside] + + +def add_guest(session: Session, actor: User, project_id: int, user_id: int) -> User: + project = _managed_project(session, actor, project_id) + guest = get_user(session, user_id) + if not guest.is_active: + raise ChatError(f"{guest.full_name} is deactivated, so can't join a chat.") + if is_team(guest, project) or is_guest(session, guest, project): + raise ChatConflictError(f"{guest.full_name} is already in this chat.") + session.add(ChatGuest(project_id=project.id, user_id=guest.id)) + session.commit() + return guest + + +def remove_guest(session: Session, actor: User, project_id: int, user_id: int) -> None: + """Staff remove a Guest; a Guest may also leave the Chat themselves.""" + if actor.id == user_id: + project = writable_project(session, actor, project_id) + else: + project = _managed_project(session, actor, project_id) + guest = session.get(ChatGuest, (project.id, user_id)) + if guest is None: + raise ChatNotFoundError("This person is not a Guest in this chat.") + session.delete(guest) + session.commit() diff --git a/backend/tests/test_chat.py b/backend/tests/test_chat.py index 6047399..fddbcb7 100644 --- a/backend/tests/test_chat.py +++ b/backend/tests/test_chat.py @@ -77,7 +77,13 @@ def test_the_team_and_staff_talk_in_the_project_chat(client: TestClient): "Good luck!", ] chat = client.get(f"/api/projects/{drone}/chat", headers=student).json() - assert chat == {"project_id": drone, "title": "Drone", "read_only": False} + assert chat == { + "project_id": drone, + "title": "Drone", + "read_only": False, + "can_manage_guests": False, + "is_guest": False, + } def test_other_students_have_no_access(client: TestClient): diff --git a/backend/tests/test_chat_guests.py b/backend/tests/test_chat_guests.py new file mode 100644 index 0000000..a2fd354 --- /dev/null +++ b/backend/tests/test_chat_guests.py @@ -0,0 +1,166 @@ +from fastapi.testclient import TestClient + +from tests.conftest import add_member, create_project, ready_to_work +from tests.test_chat import logged_in_student, send, student_id, unread + + +def guests_url(project_id: int) -> str: + return f"/api/projects/{project_id}/chat/guests" + + +def people(client: TestClient, headers, project_id: int) -> list[tuple[str, str]]: + response = client.get(f"/api/projects/{project_id}/chat/people", headers=headers) + assert response.status_code == 200, response.text + return [(person["name"], person["kind"]) for person in response.json()] + + +def user_id(client: TestClient, superuser, name: str) -> int: + users = client.get("/api/users", headers=superuser).json() + return next(u["id"] for u in users if f"{u['first_name']} {u['last_name']}" == name) + + +def test_staff_add_a_guest_who_then_reads_writes_and_leaves(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + add_member(client, teacher, drone, student_id(client, teacher)) + lotte = logged_in_student(client, superuser, "Lotte") + lotte_id = user_id(client, superuser, "Lotte Peeters") + send(client, teacher, drone, "Before Lotte joined") + + candidates = client.get(f"{guests_url(drone)}/candidates", headers=teacher).json() + added = client.post(guests_url(drone), json={"user_id": lotte_id}, headers=teacher) + + # By last name; the Product Owner and the Member are already in. + assert [c["name"] for c in candidates] == ["Superuser Account", "Lotte Peeters"] + assert added.status_code == 201 + assert people(client, lotte, drone) == [ + ("Teacher Account", "Product Owner"), + ("Student Account", "Member"), + ("Lotte Peeters", "Guest"), + ] + chat = client.get(f"/api/projects/{drone}/chat", headers=lotte).json() + assert chat["is_guest"] is True and chat["can_manage_guests"] is False + # A Guest sees the whole history and counts the chat as theirs. + assert unread(client, lotte) == {"count": 1, "has_chat": True} + assert send(client, lotte, drone, "Hi, I'm helping out").status_code == 201 + + left = client.delete(f"{guests_url(drone)}/{lotte_id}", headers=lotte) + + assert left.status_code == 204 + assert send(client, lotte, drone, "Still here?").status_code == 403 + assert unread(client, lotte) == {"count": 0, "has_chat": False} + + +def test_members_cannot_add_or_remove_guests(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + add_member(client, teacher, drone, student_id(client, teacher)) + student = ready_to_work(client, "student") + logged_in_student(client, superuser, "Lotte") + lotte_id = user_id(client, superuser, "Lotte Peeters") + + refused = client.post( + guests_url(drone), json={"user_id": lotte_id}, headers=student + ) + client.post(guests_url(drone), json={"user_id": lotte_id}, headers=superuser) + removal = client.delete(f"{guests_url(drone)}/{lotte_id}", headers=student) + + assert refused.status_code == 403 + assert refused.json()["detail"] == ( + "Only the Product Owner, Teachers and the Superuser add or remove Guests." + ) + assert removal.status_code == 403 + removed = client.delete(f"{guests_url(drone)}/{lotte_id}", headers=superuser) + assert removed.status_code == 204 + assert ("Lotte Peeters", "Guest") not in people(client, teacher, drone) + + +def test_who_cannot_be_added(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + member = student_id(client, teacher) + add_member(client, teacher, drone, member) + logged_in_student(client, superuser, "Lotte") + lotte_id = user_id(client, superuser, "Lotte Peeters") + client.post(f"/api/users/{lotte_id}/deactivate", headers=superuser) + + as_member = client.post( + guests_url(drone), json={"user_id": member}, headers=teacher + ) + deactivated = client.post( + guests_url(drone), json={"user_id": lotte_id}, headers=teacher + ) + teacher_id = user_id(client, superuser, "Teacher Account") + product_owner = client.post( + guests_url(drone), json={"user_id": teacher_id}, headers=superuser + ) + + assert as_member.status_code == 409 + assert as_member.json()["detail"] == "Student Account is already in this chat." + assert deactivated.status_code == 422 + assert product_owner.status_code == 409 + + +def test_a_teacher_counts_guest_chats_as_theirs(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + robot = create_project(client, teacher, "Robot") + superuser_id = user_id(client, superuser, "Superuser Account") + send(client, teacher, drone, "For the team") + send(client, teacher, robot, "Also for a team") + + before = client.get("/api/chats", params={"mine": True}, headers=superuser).json() + client.post(guests_url(drone), json={"user_id": superuser_id}, headers=teacher) + after = client.get("/api/chats", params={"mine": True}, headers=superuser).json() + + assert before == [] + assert [(c["title"], c["unread"]) for c in after] == [("Drone", 1)] + assert unread(client, superuser)["count"] == 1 + # Without the filter, every chat is still listed. + assert len(client.get("/api/chats", headers=superuser).json()) == 2 + + +def test_a_student_guest_without_a_project_has_a_chat(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + robot = create_project(client, teacher, "Robot") + add_member(client, teacher, robot, student_id(client, teacher)) + student = ready_to_work(client, "student") + lotte = logged_in_student(client, superuser, "Lotte") + lotte_id = user_id(client, superuser, "Lotte Peeters") + student_user = student_id(client, teacher) + + assert unread(client, lotte)["has_chat"] is False + client.post(guests_url(drone), json={"user_id": lotte_id}, headers=teacher) + client.post(guests_url(drone), json={"user_id": student_user}, headers=teacher) + + assert [c["title"] for c in client.get("/api/chats", headers=lotte).json()] == [ + "Drone" + ] + # A Member of Robot who is also a Guest in Drone has both chats. + assert [c["title"] for c in client.get("/api/chats", headers=student).json()] == [ + "Drone", + "Robot", + ] + + +def test_archiving_ends_guest_access(client: TestClient): + teacher = ready_to_work(client, "teacher") + superuser = ready_to_work(client, "superuser") + drone = create_project(client, teacher, "Drone") + lotte = logged_in_student(client, superuser, "Lotte") + lotte_id = user_id(client, superuser, "Lotte Peeters") + client.post(guests_url(drone), json={"user_id": lotte_id}, headers=teacher) + + client.post(f"/api/projects/{drone}/archive", headers=teacher) + + response = client.get(f"/api/projects/{drone}/chat/messages", headers=lotte) + assert response.status_code == 403 + assert people(client, teacher, drone) == [("Teacher Account", "Product Owner")] + added = client.post(guests_url(drone), json={"user_id": lotte_id}, headers=teacher) + assert added.status_code == 409 diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index c564dc6..5a82ddb 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -243,6 +243,7 @@ Grilling Q1–Q22 with the team, spec #91 and tickets #92–#98. Built one ticke |---|---|---|---| | New | **Chat basics:** one chat per Project (Members, Product Owner; every Teacher and the Superuser can also read and write), text messages with clickable links, new messages within ~2 s (ADR 0008), Unread badge on the Chat / Chats item, Chats screen, Chat button on a Project's page; migration `0014` | #92 | #99 | | Fixed | The restore test after a Deploy failed when the site wrote meanwhile (first seen after #99, with the new chat in use): it now compares with the row counts taken just before and after the dump, and backs up again (max 3 times) when they differ | – | #100 | +| New | **Chat Guests:** the Product Owner, a Teacher or the Superuser adds any active User to a chat (People list, Add Guest, Remove); Guests can leave; "Only my chats" on the Chats screen (on by default); a Teacher's badge also counts chats where they are a Guest; Students in several chats get a list; migration `0015` | #93 | #101 | ### 2026-10-22: team change requests diff --git a/frontend/src/pmp_frontend/api.py b/frontend/src/pmp_frontend/api.py index 02f0276..a274c53 100644 --- a/frontend/src/pmp_frontend/api.py +++ b/frontend/src/pmp_frontend/api.py @@ -697,12 +697,47 @@ async def mark_chat_read(token: str, project_id: int, message_id: int) -> None: ) -async def chats(token: str) -> list[dict[str, Any]]: - """The Chats the User sees: project_id, title, unread, mine.""" - return await _request("GET", "/api/chats", token=token) +async def chats(token: str, *, mine: bool = False) -> list[dict[str, Any]]: + """The Chats the User sees (with `mine`: only those where they are one of the + Chat people): project_id, title, unread, mine.""" + query = "?mine=true" if mine else "" + return await _request("GET", f"/api/chats{query}", token=token) async def chat_unread(token: str) -> dict[str, Any]: """The badge: `count` of Unread messages in the User's own Chats, and `has_chat`.""" return await _request("GET", "/api/chats/unread", token=token) + + +async def chat_people(token: str, project_id: int) -> list[dict[str, Any]]: + """The Product Owner, the Members, then the Guests: id, name, kind.""" + return await _request("GET", f"/api/projects/{project_id}/chat/people", token=token) + + +async def chat_guest_candidates(token: str, project_id: int) -> list[dict[str, Any]]: + """Active Users who could join the Chat as a Guest: id, name.""" + return await _request( + "GET", f"/api/projects/{project_id}/chat/guests/candidates", token=token + ) + + +async def add_chat_guest(token: str, project_id: int, user_id: int) -> dict[str, Any]: + return await _request( + "POST", + f"/api/projects/{project_id}/chat/guests", + token=token, + json={"user_id": user_id}, + ) + + +async def remove_chat_guest(token: str, project_id: int, user_id: int) -> None: + """Remove a Guest; with your own id, leave the Chat.""" + await _request( + "DELETE", f"/api/projects/{project_id}/chat/guests/{user_id}", token=token + ) + + +async def leave_chat(token: str, project_id: int) -> None: + """A Guest leaves the Chat.""" + await _request("POST", f"/api/projects/{project_id}/chat/leave", token=token) diff --git a/frontend/src/pmp_frontend/chat_page.py b/frontend/src/pmp_frontend/chat_page.py index cbe9ab9..980b01b 100644 --- a/frontend/src/pmp_frontend/chat_page.py +++ b/frontend/src/pmp_frontend/chat_page.py @@ -10,7 +10,14 @@ from starlette.responses import Response from pmp_frontend import api -from pmp_frontend.shell import ErrorMessage, guard, page_frame, role_page, token +from pmp_frontend.shell import ( + ErrorMessage, + confirm, + guard, + page_frame, + role_page, + token, +) from pmp_frontend.times import shown STAFF = {"superuser", "teacher"} @@ -120,7 +127,14 @@ async def send() -> None: box.value = "" show([sent]) - ui.label(f"Chat: {chat['title']}").classes("text-h4").mark("chat-title") + with ui.row().classes("w-full items-center"): + ui.label(f"Chat: {chat['title']}").classes("text-h4").mark("chat-title") + ui.space() + if chat["is_guest"]: + ui.button( + "Leave chat", icon="logout", on_click=lambda: _leave(project_id) + ).props("outline color=dark").mark("leave-chat") + await _people(project_id, chat) with ui.card().classes("w-full p-0"): scroll = ui.scroll_area().classes("w-full h-[60vh]").mark("chat-messages") with scroll: @@ -147,30 +161,150 @@ async def send() -> None: ui.timer(POLL_SECONDS, poll) +async def _leave(project_id: int) -> None: + question = "Leave this chat? You can only come back when someone adds you again." + if not await confirm(question, "Leave"): + return + try: + await api.leave_chat(token(), project_id) + except api.ApiError as error: + ui.notify(error.message, type="negative") + return + ui.notify("You left the chat.") + ui.navigate.to("/") + + +async def _people(project_id: int, chat: dict[str, Any]) -> None: + """Who is in the Chat (Product Owner, Members, Guests); staff add and remove + Guests here.""" + manage = chat["can_manage_guests"] + + async def remove(person: dict[str, Any]) -> None: + if not await confirm(f"Remove {person['name']} from this chat?", "Remove"): + return + try: + await api.remove_chat_guest(token(), project_id, person["id"]) + except api.ApiError as error: + ui.notify(error.message, type="negative") + return + ui.notify(f"{person['name']} was removed from the chat.") + people_list.refresh() + + async def open_add_dialog() -> None: + candidates = await api.chat_guest_candidates(token(), project_id) + + async def add() -> None: + if choice.value is None: + error.show("Choose a person first.") + return + try: + added = await api.add_chat_guest(token(), project_id, choice.value) + except api.ApiError as problem: + error.show(problem.message) + return + dialog.close() + ui.notify(f"{added['name']} joined the chat as a Guest.") + people_list.refresh() + + with ui.dialog() as dialog, ui.card().classes("w-full max-w-md"): + ui.label("Add a Guest").classes("text-h6") + ui.label( + "Guests read the whole chat and write like the team, until they " + "leave or are removed." + ).classes("text-caption") + choice = ( + ui.select( + {person["id"]: person["name"] for person in candidates}, + label="Person", + with_input=True, + ) + .classes("w-full") + .mark("guest-choice") + ) + error = ErrorMessage() + with ui.row(): + ui.button("Add", on_click=add).mark("confirm-add-guest") + ui.button("Cancel", on_click=dialog.close).props("flat color=dark") + dialog.open() + + @ui.refreshable + async def people_list() -> None: + try: + found = await api.chat_people(token(), project_id) + except api.ApiError as error: + ui.label(error.message) + return + with ( + ui.expansion(f"People ({len(found)})", icon="group") + .classes("w-full bg-white rounded-lg") + .mark("chat-people") + ): + for person in found: + with ui.row().classes("w-full items-center"): + ui.label(person["name"]).mark(f"person-{person['id']}") + ui.label(person["kind"]).classes("text-caption").style( + "color: #666666" + ) + ui.space() + if manage and person["kind"] == "Guest": + ui.button("Remove", on_click=lambda p=person: remove(p)).props( + "flat dense color=dark" + ).mark(f"remove-guest-{person['id']}") + if manage: + ui.button( + "Add Guest", icon="person_add", on_click=open_add_dialog + ).props("outline color=dark").mark("add-guest") + + await people_list() + + +def _chats_table(found: list[dict[str, Any]]) -> None: + """Chats by title with their Unread count; a row opens its Chat.""" + table = ui.table(columns=CHATS_COLUMNS, rows=found, row_key="project_id") + table.classes("w-full").mark("chats") + table.add_slot("body-cell-unread", UNREAD_CELL) + table.on( + "rowClick", + lambda event: ui.navigate.to(f"/chats/{event.args[1]['project_id']}"), + ) + + def register() -> None: @role_page("/chat", "Chat", {"student"}) async def student_chat(user: api.CurrentUser) -> None: found = await api.chats(token()) + if len(found) == 1: + await chat_view(found[0]["project_id"]) + return + ui.label("Chat").classes("text-h4") if not found: - ui.label("Chat").classes("text-h4") ui.label("You'll have a chat once you're a Member of a Project.") return - await chat_view(found[0]["project_id"]) + # Their own Project's Chat and those where they are a Guest. + _chats_table(found) @role_page("/chats", "Chats", STAFF) async def chats_page(user: api.CurrentUser) -> None: ui.label("Chats").classes("text-h4") - found = await api.chats(token()) - if not found: - ui.label("No Active Projects, so no chats yet.") - return - table = ui.table(columns=CHATS_COLUMNS, rows=found, row_key="project_id") - table.classes("w-full").mark("chats") - table.add_slot("body-cell-unread", UNREAD_CELL) - table.on( - "rowClick", - lambda event: ui.navigate.to(f"/chats/{event.args[1]['project_id']}"), - ) + only_mine = ui.switch( + "Only my chats", value=True, on_change=lambda: listed.refresh() + ).mark("only-my-chats") + + @ui.refreshable + async def listed() -> None: + found = await api.chats(token(), mine=only_mine.value) + if found: + _chats_table(found) + elif only_mine.value: + ui.label( + "You're not in any chat yet: Projects where you are the Product " + 'Owner or a Guest appear here. Turn off "Only my chats" to see ' + "every chat." + ) + else: + ui.label("No Active Projects, so no chats yet.") + + await listed() @ui.page("/chats/{project_id}") async def chat_page(project_id: int) -> Response | None: diff --git a/frontend/tests/test_chat_guests.py b/frontend/tests/test_chat_guests.py new file mode 100644 index 0000000..3537bc5 --- /dev/null +++ b/frontend/tests/test_chat_guests.py @@ -0,0 +1,106 @@ +from nicegui import ui +from nicegui.testing import User + +from pmp_frontend import api +from tests.conftest import eventually, ready_to_work +from tests.test_chat import api_token, found, navigation_targets + + +async def project_owned_by_the_teacher(token: str, title: str) -> int: + teachers = await api.teachers(token) + owner = next(t["id"] for t in teachers if t["name"] == "Teacher Account") + project = await api.create_project( + token, + { + "title": title, + "description": None, + "product_owner_id": owner, + "team_size_min": 1, + "team_size_max": 3, + }, + ) + return project["id"] + + +async def the_student(token: str) -> int: + students = await api.students(token) + return next(s["id"] for s in students if s["name"] == "Student Account") + + +async def test_a_teacher_adds_and_removes_a_guest(user: User): + superuser = await api_token("superuser") + drone = await project_owned_by_the_teacher(superuser, "Drone") + student = await the_student(superuser) + await ready_to_work(user, "teacher") + + await user.open(f"/chats/{drone}") + await user.should_see(marker="add-guest", retries=30) + user.find(marker="add-guest").click() + await user.should_see(marker="guest-choice", retries=30) + choice = user.find(marker="guest-choice").elements.pop() + assert isinstance(choice, ui.select) + choice.set_value(student) + user.find(marker="confirm-add-guest").click() + + await user.should_see("Student Account joined the chat as a Guest.", retries=30) + await user.should_see(marker=f"remove-guest-{student}", retries=30) + await user.should_see("People (2)") + + user.find(marker=f"remove-guest-{student}").click() + await user.should_see("Remove Student Account from this chat?", retries=10) + user.find(marker="confirm").click() + await user.should_see("Student Account was removed from the chat.", retries=30) + await user.should_see("People (1)", retries=30) + + +async def test_a_student_guest_chats_and_leaves(user: User): + superuser = await api_token("superuser") + drone = await project_owned_by_the_teacher(superuser, "Drone") + await api.add_chat_guest(superuser, drone, await the_student(superuser)) + await ready_to_work(user, "student") + + # A Guest without a Project still gets the Chat item, straight into the chat. + await eventually(lambda: "/chat" in navigation_targets(user), retries=100) + await user.open("/chat") + await user.should_see("Chat: Drone", retries=30) + assert not found(user, "add-guest") + + user.find(marker="leave-chat").click() + await user.should_see("Leave this chat?", retries=10) + user.find(marker="confirm").click() + + await user.should_see("You left the chat.", retries=30) + assert await api.chats(superuser) == [ + {"project_id": drone, "title": "Drone", "unread": 0, "mine": False} + ] + + +async def test_only_my_chats_is_on_by_default(user: User): + teacher = await api_token("teacher") + await project_owned_by_the_teacher(teacher, "Drone") + await ready_to_work(user, "superuser") + + await user.open("/chats") + await user.should_see("You're not in any chat yet", retries=30) + switch = user.find(marker="only-my-chats").elements.pop() + assert isinstance(switch, ui.switch) + switch.set_value(False) + + await user.should_see(marker="chats", retries=30) + table = user.find(marker="chats").elements.pop() + assert [row["title"] for row in table.rows] == ["Drone"] + + +async def test_a_student_in_two_chats_chooses_one(user: User): + superuser = await api_token("superuser") + drone = await project_owned_by_the_teacher(superuser, "Drone") + robot = await project_owned_by_the_teacher(superuser, "Robot") + student = await the_student(superuser) + await api.add_member(superuser, robot, student) + await api.add_chat_guest(superuser, drone, student) + await ready_to_work(user, "student") + + await user.open("/chat") + await user.should_see(marker="chats", retries=30) + table = user.find(marker="chats").elements.pop() + assert [row["title"] for row in table.rows] == ["Drone", "Robot"]