From 18a8c3d5df198331ab8fc0c68a574f1880955aba Mon Sep 17 00:00:00 2001 From: Sergio Rayo Date: Sun, 13 Sep 2026 14:11:55 +0200 Subject: [PATCH] Allow party invitations to be retried and expire them consistently (#368) An invitation rejected by the server, for example because the recipient already belongs to another party, left the sender's invite button disabled. Even after the recipient left that party, the sender had to leave and recreate their own party to invite them again. Server-side, invitation cleanup iterated a PartyComponent view that is never populated, invitations were keyed by Player pointers, and acceptance did not check the deadline. UI: keep per-player invite state with a short retry cooldown instead of a permanent flag, expire incoming invitations using the remaining time reported by the client, and clear them on join, leave and disconnect. Native client: forward the remaining time, drop invites that are already expired or arrive while in a party, and clear invitations on join and party teardown. Server: key invitations by player id, expire them from the player list, check the deadline when accepting and consume the invitation once, and revoke a player's outgoing invitations when they disconnect or stop leading a party. No new network messages. Adds server component tests (TPServerTests target behind the server_tests xmake option) and extends the party browser tests. --- Code/client/Services/Generic/PartyService.cpp | 12 +- Code/server/Components/PartyComponent.h | 25 +- Code/server/Services/PartyService.cpp | 46 +- Code/server/Services/PartyService.h | 1 + .../playwright/specs/group.e2e-spec.ts | 423 +++++++++++++++--- .../src/app/mock/skyrimtogether.mock.ts | 4 +- Code/skyrim_ui/src/app/models/party-invite.ts | 4 + .../src/app/services/client.service.ts | 7 +- .../src/app/services/group.service.ts | 15 +- .../src/app/services/player-list.service.ts | 190 ++++++-- Code/skyrim_ui/src/typings.d.ts | 5 +- Code/tests/server/party_invitations.cpp | 62 +++ Code/tests/server/xmake.lua | 17 + Code/tests/xmake.lua | 10 + 14 files changed, 692 insertions(+), 129 deletions(-) create mode 100644 Code/skyrim_ui/src/app/models/party-invite.ts create mode 100644 Code/tests/server/party_invitations.cpp create mode 100644 Code/tests/server/xmake.lua diff --git a/Code/client/Services/Generic/PartyService.cpp b/Code/client/Services/Generic/PartyService.cpp index ed6bbed0a..e9bebf137 100644 --- a/Code/client/Services/Generic/PartyService.cpp +++ b/Code/client/Services/Generic/PartyService.cpp @@ -58,7 +58,8 @@ void PartyService::CreateInvite(const uint32_t aPlayerId) const noexcept void PartyService::AcceptInvite(const uint32_t aInviterId) const noexcept { - if (!m_invitations.contains(aInviterId)) + const auto itor = m_invitations.find(aInviterId); + if (!m_transport.IsConnected() || m_inParty || itor == m_invitations.end() || itor->second <= m_transport.GetClock().GetCurrentTick()) return; PartyAcceptInviteRequest request; @@ -92,7 +93,7 @@ void PartyService::OnUpdate(const UpdateEvent& acEvent) noexcept auto itor = std::begin(m_invitations); while (itor != std::end(m_invitations)) { - if (itor->second < cCurrentTick) + if (itor->second <= cCurrentTick) itor = m_invitations.erase(itor); else ++itor; @@ -140,12 +141,17 @@ void PartyService::OnPartyInfo(const NotifyPartyInfo& acPartyInfo) noexcept void PartyService::OnPartyInvite(const NotifyPartyInvite& acPartyInvite) noexcept { + const auto cCurrentTick = m_transport.GetClock().GetCurrentTick(); + if (m_inParty || acPartyInvite.ExpiryTick <= cCurrentTick) + return; + spdlog::debug("[PartyService]: Got party invite from {}", acPartyInvite.InviterId); m_invitations[acPartyInvite.InviterId] = acPartyInvite.ExpiryTick; auto pArguments = CefListValue::Create(); pArguments->SetInt(0, acPartyInvite.InviterId); + pArguments->SetDouble(1, static_cast(acPartyInvite.ExpiryTick - cCurrentTick)); m_world.GetOverlayService().GetOverlayApp()->ExecuteAsync("partyInviteReceived", pArguments); } @@ -157,6 +163,7 @@ void PartyService::OnPartyJoined(const NotifyPartyJoined& acPartyJoined) noexcep m_isLeader = acPartyJoined.IsLeader; m_leaderPlayerId = acPartyJoined.LeaderPlayerId; m_partyMembers = acPartyJoined.PlayerIds; + m_invitations.clear(); m_world.GetDispatcher().trigger(PartyJoinedEvent(m_isLeader)); } @@ -176,4 +183,5 @@ void PartyService::DestroyParty() noexcept m_isLeader = false; m_leaderPlayerId = -1; m_partyMembers.clear(); + m_invitations.clear(); } diff --git a/Code/server/Components/PartyComponent.h b/Code/server/Components/PartyComponent.h index aaf5eaf44..a4b455afa 100644 --- a/Code/server/Components/PartyComponent.h +++ b/Code/server/Components/PartyComponent.h @@ -8,6 +8,29 @@ struct PartyComponent { PartyComponent() {} + void ExpireInvitations(uint64_t aCurrentTick) noexcept + { + auto itor = Invitations.begin(); + while (itor != Invitations.end()) + { + if (itor->second <= aCurrentTick) + itor = Invitations.erase(itor); + else + ++itor; + } + } + + bool TryConsumeInvitation(uint32_t aInviterId, uint64_t aCurrentTick) noexcept + { + const auto itor = Invitations.find(aInviterId); + if (itor == Invitations.end()) + return false; + + const bool isValid = itor->second > aCurrentTick; + Invitations.erase(itor); + return isValid; + } + std::optional JoinedPartyId; - TiltedPhoques::Map Invitations; + TiltedPhoques::Map Invitations; }; diff --git a/Code/server/Services/PartyService.cpp b/Code/server/Services/PartyService.cpp index 5fc357781..a818f0af2 100644 --- a/Code/server/Services/PartyService.cpp +++ b/Code/server/Services/PartyService.cpp @@ -79,22 +79,9 @@ void PartyService::OnUpdate(const UpdateEvent& acEvent) noexcept // Only expire once every 10 seconds m_nextInvitationExpire = cCurrentTick + 10000; - auto view = m_world.view(); - for (auto entity : view) + for (Player* pPlayer : m_world.GetPlayerManager()) { - auto& partyComponent = view.get(entity); - auto itor = std::begin(partyComponent.Invitations); - while (itor != std::end(partyComponent.Invitations)) - { - if (itor->second < cCurrentTick) - { - itor = partyComponent.Invitations.erase(itor); - } - else - { - ++itor; - } - } + pPlayer->GetParty().ExpireInvitations(cCurrentTick); } } @@ -158,6 +145,9 @@ void PartyService::OnPartyChangeLeader(const PacketEventGetId() == pNewLeader->GetId()) { + if (party.LeaderPlayerId != pNewLeader->GetId()) + RevokeInvitations(party.LeaderPlayerId); + party.LeaderPlayerId = pPlayer->GetId(); spdlog::debug("[PartyService]: Changed party leader to {}, updating party members.", party.LeaderPlayerId); BroadcastPartyInfo(*inviterPartyComponent.JoinedPartyId); @@ -271,7 +261,7 @@ void PartyService::OnPartyInvite(const PacketEvent& acPacket // Expire in 60 seconds const auto cExpiryTick = GameServer::Get()->GetTick() + 60000; - inviteePartyComponent.Invitations[pInviter] = cExpiryTick; + inviteePartyComponent.Invitations[pInviter->GetId()] = cExpiryTick; NotifyPartyInvite notification; notification.InviterId = pInviter->GetId(); @@ -297,11 +287,6 @@ void PartyService::OnPartyAcceptInvite(const PacketEventGetParty(); auto& selfPartyComponent = pSelf->GetParty(); - // Check if we have this invitation so people don't invite themselves - if (selfPartyComponent.Invitations.count(pInviter) == 0) - return; - - spdlog::debug("[PartyService]: Invite found, processing."); if (!inviterPartyComponent.JoinedPartyId) // Ensure inviter is in a party otherwise break { spdlog::debug("[PartyService]: Inviter not in party. Cancelling."); @@ -324,6 +309,13 @@ void PartyService::OnPartyAcceptInvite(const PacketEventGetId(), GameServer::Get()->GetTick())) + { + spdlog::debug("[PartyService]: Invitation missing or expired, cancelling."); + return; + } + party.Members.push_back(pSelf); selfPartyComponent.JoinedPartyId = partyId; @@ -347,6 +339,8 @@ void PartyService::OnPlayerLeave(const PlayerLeaveEvent& acEvent) noexcept void PartyService::RemovePlayerFromParty(Player* apPlayer) noexcept { auto* pPartyComponent = &apPlayer->GetParty(); + pPartyComponent->Invitations.clear(); + RevokeInvitations(apPlayer->GetId()); spdlog::debug("[PartyService]: Removing player from party."); if (pPartyComponent->JoinedPartyId) @@ -381,6 +375,14 @@ void PartyService::RemovePlayerFromParty(Player* apPlayer) noexcept } } +void PartyService::RevokeInvitations(uint32_t aInviterId) noexcept +{ + for (Player* pPlayer : m_world.GetPlayerManager()) + { + pPlayer->GetParty().Invitations.erase(aInviterId); + } +} + void PartyService::BroadcastPlayerList(Player* apPlayer) const noexcept { auto pIgnoredPlayer = apPlayer; @@ -431,6 +433,8 @@ void PartyService::BroadcastPartyInfo(uint32_t aPartyId) const noexcept void PartyService::SendPartyJoinedEvent(Party& aParty, Player* aPlayer) noexcept { + aPlayer->GetParty().Invitations.clear(); + NotifyPartyJoined joinedMessage; joinedMessage.LeaderPlayerId = aParty.LeaderPlayerId; joinedMessage.IsLeader = aParty.LeaderPlayerId == aPlayer->GetId(); diff --git a/Code/server/Services/PartyService.h b/Code/server/Services/PartyService.h index e1803ca19..152e01dfc 100644 --- a/Code/server/Services/PartyService.h +++ b/Code/server/Services/PartyService.h @@ -47,6 +47,7 @@ struct PartyService void OnPartyChangeLeader(const PacketEvent& acPacket) noexcept; void OnPartyKick(const PacketEvent& acPacket) noexcept; void RemovePlayerFromParty(Player* apPlayer) noexcept; + void RevokeInvitations(uint32_t aInviterId) noexcept; void BroadcastPlayerList(Player* apPlayer = nullptr) const noexcept; void BroadcastPartyInfo(uint32_t aPartyId) const noexcept; diff --git a/Code/skyrim_ui/playwright/specs/group.e2e-spec.ts b/Code/skyrim_ui/playwright/specs/group.e2e-spec.ts index 750ddf13f..dc584ed97 100644 --- a/Code/skyrim_ui/playwright/specs/group.e2e-spec.ts +++ b/Code/skyrim_ui/playwright/specs/group.e2e-spec.ts @@ -1,84 +1,385 @@ import { createTest, expect } from '@ngx-playwright/test'; +import type { Page } from '@ngx-playwright/test'; import type { MockPlayer } from '../../src/app/mock/mock-player.js'; -import { createClassXPathSelector } from '../helpers/selector.js'; - import { ApplicationScreen } from '../screens/main-screen.js'; - const test = createTest(ApplicationScreen); -test.describe('Group', () => { +async function connect(page: Page) { + await page + .locator('.app-root-menu') + .getByRole('button', { name: 'Connect', exact: true }) + .click(); + await page.locator('app-connect input').nth(0).fill('test'); + await page.locator('app-connect input').nth(1).fill('test'); + await page.locator('app-connect app-action-buttons button').first().click(); + await expect( + page + .locator('.app-root-menu') + .getByRole('button', { name: 'Disconnect', exact: true }), + ).toBeVisible(); +} + +async function openPartyMenu(page: Page) { + if (!(await page.locator('app-player-manager').count())) { + await page + .locator('.app-root-menu') + .getByRole('button', { name: 'Player Manager', exact: true }) + .click(); + } + await page + .locator('app-player-manager') + .getByRole('button', { name: 'Party Menu', exact: true }) + .click(); +} + +async function openPlayerList(page: Page) { + await page + .locator('app-player-manager') + .getByRole('button', { name: 'Player List', exact: true }) + .click(); +} + +async function closePlayerManager(page: Page) { + await page + .locator('app-player-manager') + .getByRole('button', { name: 'Back', exact: true }) + .click(); + await expect(page.locator('app-player-manager')).toHaveCount(0); +} + +async function addPlayer(page: Page): Promise { + return page.evaluate('skyrimtogether.addMockPlayer()'); +} + +function inviteButton(page: Page, player: MockPlayer) { + return page + .locator('.player-list tbody tr') + .filter({ hasText: player.name }) + .getByRole('button', { name: 'Invite', exact: true }); +} +function receivedInvite(page: Page, player: MockPlayer) { + return page.locator('app-party-menu').getByRole('button', { + name: `Accept invite from ${player.name}`, + exact: true, + }); +} + +async function receiveInvite( + page: Page, + player: MockPlayer, + expiresInMs: number, +) { + await page.evaluate( + ({ id, expiresInMs }) => { + const mock = (window as any).skyrimtogether; + mock.startPlayerMockParty(id); + mock.emit('partyInviteReceived', id, expiresInMs); + }, + { id: player.id, expiresInMs }, + ); +} + +test.describe('Group', () => { test.beforeEach(async ({ page }) => { await page.waitForSelector('.app-root-controls', { state: 'attached' }); await page.press('body', 'F2'); await page.waitForSelector('.app-root-controls', { state: 'visible' }); + await connect(page); - // connect - await page.click(`//app-window${ createClassXPathSelector('app-root-menu') }/button[1]`); - await page.waitForSelector('//app-connect'); - await page.locator('//app-connect/div[1]/input[1]').fill('test'); - await page.locator('//app-connect/div[1]/input[2]').fill('test'); - await page.click('//app-connect/div[1]/app-action-buttons[1]/button[1]'); - await expect(page.locator(`//app-window${ createClassXPathSelector('app-root-menu') }/button[1]`)).toHaveText(/Disconnect/); + // Observe calls at the native bridge, without granting the UI an invitation. + await page.evaluate(() => { + const mock = (window as any).skyrimtogether; + for (const method of ['createPartyInvite', 'acceptPartyInvite']) { + const original = mock[method].bind(mock); + mock[`${method}Calls`] = []; + mock[method] = (id: number) => { + mock[`${method}Calls`].push(id); + original(id); + }; + } + }); }); test('Invite & Kick', async ({ page }) => { - const player = await page.evaluate('skyrimtogether.addMockPlayer()') as MockPlayer; - - // open player manager - await page.click(`//app-window${ createClassXPathSelector('app-root-menu') }/button[2]`); - await page.click(`//app-player-manager/div[1]/button[2]`); - - // launch party - await expect(await page.locator('.player-list-table').count()).toBe(0); - await page.click(`//app-party-menu[1]/div[1]/button[1]`); - await expect(await page.locator('.player-list-table').count()).toBe(0); - await expect(page.locator(`//app-party-menu/div[1]/span[1]`)).toHaveText(/There is nobody in your party yet/); - - // open playerlist and invite player - await page.click(`//app-player-manager/div[1]/button[1]`); - await page.click(`//div${ createClassXPathSelector('player-list') }/table[1]/tbody[1]/tr[1]/td[4]/button[1]`); - await expect(page.locator(`//div${ createClassXPathSelector('player-list') }/table[1]/tbody[1]/tr[1]/td[4]/button[1]`)).toBeDisabled(); - - // accept invite - await page.evaluate(`skyrimtogether.accteptMockPlayerInvite(${ player.id })`); - await expect(page.locator(`//div${ createClassXPathSelector('player-list') }/table[1]/tbody[1]/tr[1]/td[4]/button[1]`)).toBeDisabled(); - - // check member list - await page.click(`//app-player-manager/div[1]/button[2]`); - await expect(await page.locator(`//div${ createClassXPathSelector('member-list') }/table[1]/tbody[1]/tr`).count()).toBe(1); - await expect(page.locator(`//div${ createClassXPathSelector('member-list') }/table[1]/tbody[1]/tr[1]/td[1]`)).toHaveText(`${ player.level }`); - await expect(page.locator(`//div${ createClassXPathSelector('member-list') }/table[1]/tbody[1]/tr[1]/td[2]`)).toHaveText(player.name); - await expect(page.locator(`//div${ createClassXPathSelector('member-list') }/table[1]/tbody[1]/tr[1]/td[3]`)).toHaveText(player.cellName); - - // kick member - await page.click(`//div${ createClassXPathSelector('member-list') }/table[1]/tbody[1]/tr[1]/td[4]/button[2]`); - await expect(await page.locator('.player-list-table').count()).toBe(0); - - // check invite button - await page.click(`//app-player-manager/div[1]/button[1]`); - await expect(page.locator(`//div${ createClassXPathSelector('player-list') }/table[1]/tbody[1]/tr[1]/td[4]/button[1]`)).toBeEnabled(); + const player = await addPlayer(page); + await openPartyMenu(page); + await page + .getByRole('button', { name: 'Launch party', exact: true }) + .click(); + await expect(page.locator('app-party-menu .no-players')).toContainText( + 'There is nobody in your party yet', + ); + + await openPlayerList(page); + await inviteButton(page, player).click(); + await expect(inviteButton(page, player)).toBeDisabled(); + await page.evaluate(`skyrimtogether.accteptMockPlayerInvite(${player.id})`); + await expect(inviteButton(page, player)).toBeDisabled(); + + await openPartyMenu(page); + const member = page.locator('.member-list tbody tr'); + await expect(member).toHaveCount(1); + await expect(member.locator('td').nth(0)).toHaveText(`${player.level}`); + await expect(member.locator('td').nth(1)).toHaveText(player.name); + await expect(member.locator('td').nth(2)).toHaveText(player.cellName); + await member.getByRole('button', { name: 'Kick', exact: true }).click(); + await expect(member).toHaveCount(0); + + await openPlayerList(page); + await expect(inviteButton(page, player)).toBeEnabled(); + await inviteButton(page, player).click(); + await expect( + page.evaluate('skyrimtogether.createPartyInviteCalls'), + ).resolves.toEqual([player.id, player.id]); }); - test('Launch & Leave Party', async ({ page }) => { - // open player manager - await page.click(`//app-window${ createClassXPathSelector('app-root-menu') }/button[2]`); - await page.click(`//app-player-manager/div[1]/button[2]`); + test('Ignored invites can be retried after a short per-player cooldown', async ({ + page, + }) => { + const player = await addPlayer(page); + const otherPlayer = await addPlayer(page); + await openPartyMenu(page); + await page + .getByRole('button', { name: 'Launch party', exact: true }) + .click(); + await openPlayerList(page); - // launch party - await expect(page.locator(`//app-party-menu/div[1]/button[1]`)).toHaveText(/Launch party/); - await page.click(`//app-party-menu/div[1]/button[1]`); + await inviteButton(page, player).click(); + await expect(inviteButton(page, player)).toBeDisabled(); + await expect(inviteButton(page, otherPlayer)).toBeEnabled(); + await expect(inviteButton(page, player)).toBeEnabled({ timeout: 4500 }); + await inviteButton(page, player).click(); + await expect(inviteButton(page, player)).toBeDisabled(); + await expect( + page.evaluate('skyrimtogether.createPartyInviteCalls'), + ).resolves.toEqual([player.id, player.id]); + }); + + test('Received invitation expires and its stale popup cannot accept it', async ({ + page, + }) => { + const player = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 1500); + await expect(receivedInvite(page, player)).toBeVisible(); + const popup = page + .locator('app-notification-popup') + .filter({ hasText: player.name }); + await closePlayerManager(page); + // Hover pauses the presentation timer, but must not extend the invitation. + await popup.hover(); + await page.waitForTimeout(1600); + await popup.getByRole('button', { name: 'Accept', exact: true }).click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([]); + await openPartyMenu(page); + await expect(receivedInvite(page, player)).toHaveCount(0); + await expect( + page.getByRole('button', { name: 'Launch party', exact: true }), + ).toBeVisible(); + }); + + test('Renewing an invite cancels its old deadline and invalidates its old popup', async ({ + page, + }) => { + const player = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 1800); + const popups = page + .locator('app-notification-popup') + .filter({ hasText: player.name }); + await closePlayerManager(page); + await popups.first().hover(); + await receiveInvite(page, player, 6000); + await expect(popups).toHaveCount(2); + await popups + .first() + .getByRole('button', { name: 'Accept', exact: true }) + .click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([]); + await openPartyMenu(page); + await page.waitForTimeout(1900); + await expect(receivedInvite(page, player)).toBeVisible(); + await receivedInvite(page, player).click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([player.id]); + await expect( + page.getByRole('button', { name: 'Leave party', exact: true }), + ).toBeVisible(); + }); + + test('Joining and leaving clears every old received invite', async ({ + page, + }) => { + const player = await addPlayer(page); + const otherPlayer = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 60000); + await receiveInvite(page, otherPlayer, 60000); + await receivedInvite(page, player).click(); + await page + .getByRole('button', { name: 'Leave party', exact: true }) + .click(); + await expect(receivedInvite(page, player)).toHaveCount(0); + await expect(receivedInvite(page, otherPlayer)).toHaveCount(0); + const stalePopup = page + .locator('app-notification-popup') + .filter({ hasText: otherPlayer.name }); + await closePlayerManager(page); + await stalePopup + .getByRole('button', { name: 'Accept', exact: true }) + .click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([player.id]); + + await receiveInvite(page, otherPlayer, 60000); + await openPartyMenu(page); + await receivedInvite(page, otherPlayer).click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([player.id, otherPlayer.id]); + }); - // leave party - await expect(page.locator(`//app-party-menu/button[1]`)).toHaveText(/Leave party/); - await page.click(`//app-party-menu/button[1]`); + test('A rejected acceptance preserves another valid invitation', async ({ + page, + }) => { + const player = await addPlayer(page); + const otherPlayer = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 60000); + await receiveInvite(page, otherPlayer, 60000); + await page.evaluate(rejectedId => { + const mock = (window as any).skyrimtogether; + const accept = mock.acceptPartyInvite.bind(mock); + mock.acceptPartyInvite = (id: number) => { + if (id === rejectedId) { + // A stale server invite produces no successful partyInfo response. + mock.acceptPartyInviteCalls.push(id); + return; + } + accept(id); + }; + }, player.id); - // check if the launch button reappeared - await expect(page.locator(`//app-party-menu/div[1]/button[1]`)).toHaveText(/Launch party/); + await receivedInvite(page, player).click(); + await expect(receivedInvite(page, player)).toHaveCount(0); + await expect(receivedInvite(page, otherPlayer)).toBeVisible(); + await receivedInvite(page, otherPlayer).click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([player.id, otherPlayer.id]); + await expect( + page.getByRole('button', { name: 'Leave party', exact: true }), + ).toBeVisible(); + }); + + test('Leaving and relaunching permits an immediate fresh invite', async ({ + page, + }) => { + const player = await addPlayer(page); + await openPartyMenu(page); + await page + .getByRole('button', { name: 'Launch party', exact: true }) + .click(); + await openPlayerList(page); + await inviteButton(page, player).click(); + await openPartyMenu(page); + await page + .getByRole('button', { name: 'Leave party', exact: true }) + .click(); + await page + .getByRole('button', { name: 'Launch party', exact: true }) + .click(); + await openPlayerList(page); + await expect(inviteButton(page, player)).toBeEnabled(); + await inviteButton(page, player).click(); + await expect(inviteButton(page, player)).toBeDisabled(); + await expect( + page.evaluate('skyrimtogether.createPartyInviteCalls'), + ).resolves.toEqual([player.id, player.id]); + }); + + test('Disconnecting clears invites and rejects callbacks from the previous connection', async ({ + page, + }) => { + const player = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 60000); + await page + .locator('app-player-manager') + .getByRole('button', { name: 'Back', exact: true }) + .click(); + await page + .locator('.app-root-menu') + .getByRole('button', { name: 'Disconnect', exact: true }) + .click(); + await page + .locator('app-disconnect') + .getByRole('button', { name: 'Proceed', exact: true }) + .click(); + await connect(page); + await openPartyMenu(page); + await expect(receivedInvite(page, player)).toHaveCount(0); + await receiveInvite(page, player, 60000); + const popups = page + .locator('app-notification-popup') + .filter({ hasText: player.name }); + await closePlayerManager(page); + await popups + .first() + .getByRole('button', { name: 'Accept', exact: true }) + .click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([]); + await openPartyMenu(page); + await receivedInvite(page, player).click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([player.id]); + }); + + test('A disconnected inviter cannot be accepted from its popup', async ({ + page, + }) => { + const player = await addPlayer(page); + await openPartyMenu(page); + await receiveInvite(page, player, 60000); + await page.evaluate(`skyrimtogether.disconnectMockPlayer(${player.id})`); + await expect(receivedInvite(page, player)).toHaveCount(0); + await closePlayerManager(page); + await page + .locator('app-notification-popup') + .filter({ hasText: player.name }) + .getByRole('button', { name: 'Accept', exact: true }) + .click(); + await expect( + page.evaluate('skyrimtogether.acceptPartyInviteCalls'), + ).resolves.toEqual([]); + }); - // close the player manager - await page.click(`//app-player-manager/div${ createClassXPathSelector('actions') }[1]//button[1]`); - await page.waitForSelector('//app-player-manager', { state: 'detached' }); + test('Launch & Leave Party', async ({ page }) => { + await openPartyMenu(page); + await page + .getByRole('button', { name: 'Launch party', exact: true }) + .click(); + await page + .getByRole('button', { name: 'Leave party', exact: true }) + .click(); + await expect( + page.getByRole('button', { name: 'Launch party', exact: true }), + ).toBeVisible(); + await page + .locator('app-player-manager') + .getByRole('button', { name: 'Back', exact: true }) + .click(); + await expect(page.locator('app-player-manager')).toHaveCount(0); }); }); diff --git a/Code/skyrim_ui/src/app/mock/skyrimtogether.mock.ts b/Code/skyrim_ui/src/app/mock/skyrimtogether.mock.ts index 50f2d4815..7cb92084e 100644 --- a/Code/skyrim_ui/src/app/mock/skyrimtogether.mock.ts +++ b/Code/skyrim_ui/src/app/mock/skyrimtogether.mock.ts @@ -310,9 +310,9 @@ export class SkyrimtogetherMock extends EventEmitter implements SkyrimTogether { ); } - inviteToPlayerMockParty(playerId: number) { + inviteToPlayerMockParty(playerId: number, expiresInMs = 60000) { playerStore.update(updateEntities(playerId, { invitedLocalPlayer: true })); - this.emit('partyInviteReceived', playerId); + this.emit('partyInviteReceived', playerId, expiresInMs); } startPlayerMockParty(playerId: number) { diff --git a/Code/skyrim_ui/src/app/models/party-invite.ts b/Code/skyrim_ui/src/app/models/party-invite.ts new file mode 100644 index 000000000..5ba91886d --- /dev/null +++ b/Code/skyrim_ui/src/app/models/party-invite.ts @@ -0,0 +1,4 @@ +export interface PartyInvite { + inviterId: number; + expiresInMs: number; +} diff --git a/Code/skyrim_ui/src/app/services/client.service.ts b/Code/skyrim_ui/src/app/services/client.service.ts index 2dc68f759..775b22c2c 100644 --- a/Code/skyrim_ui/src/app/services/client.service.ts +++ b/Code/skyrim_ui/src/app/services/client.service.ts @@ -4,6 +4,7 @@ import { AsyncSubject, BehaviorSubject, ReplaySubject, Subject } from 'rxjs'; import { environment } from '../../environments/environment'; import { Debug } from '../models/debug'; import { PartyInfo } from '../models/party-info'; +import { PartyInvite } from '../models/party-invite'; import { Player } from '../models/player'; import { ChatService } from './chat.service'; import { ErrorEvents, ErrorService } from './error.service'; @@ -53,7 +54,7 @@ export class ClientService implements OnDestroy { public partyLeftChange = new Subject(); /** Connect party invite received. */ - public partyInviteReceivedChange = new Subject(); + public partyInviteReceivedChange = new Subject(); /** Disconnect player to server change. */ public playerDisconnectedChange = new Subject(); @@ -619,7 +620,7 @@ export class ClientService implements OnDestroy { }); } - private onPartyInviteReceived(inviterId: number) { + private onPartyInviteReceived(inviterId: number, expiresInMs = 60000) { if (environment.game) { console.log( `%conPartyInviteReceived`, @@ -628,7 +629,7 @@ export class ClientService implements OnDestroy { ); } this.zone.run(() => { - this.partyInviteReceivedChange.next(inviterId); + this.partyInviteReceivedChange.next({ inviterId, expiresInMs }); }); } } diff --git a/Code/skyrim_ui/src/app/services/group.service.ts b/Code/skyrim_ui/src/app/services/group.service.ts index 706a19f93..77201e228 100644 --- a/Code/skyrim_ui/src/app/services/group.service.ts +++ b/Code/skyrim_ui/src/app/services/group.service.ts @@ -122,13 +122,6 @@ export class GroupService implements OnDestroy { for (let id of partyInfo.playerIds) { group.members.push(id); - - for (const player of playerList.players) { - if (player.id === id) { - player.hasBeenInvited = false; - break; - } - } } group.owner = partyInfo.leaderId; @@ -147,8 +140,6 @@ export class GroupService implements OnDestroy { const group = this.createGroup(this.group.getValue()); if (group) { - this.playerListService.resetHasBeenInvitedFlags(); - group.isEnabled = false; group.owner = undefined; group.members.splice(0); @@ -232,6 +223,7 @@ export class GroupService implements OnDestroy { if (group) { this.soundService.play(Sound.Focus); this.loadingService.setLoading(true); + this.playerListService.resetPartyInvitations(); this.clientService.launchParty(); group.isEnabled = true; @@ -248,6 +240,7 @@ export class GroupService implements OnDestroy { } this.soundService.play(Sound.Ok); + this.playerListService.resetPartyState(); this.clientService.leaveParty(); group.isEnabled = false; @@ -260,7 +253,7 @@ export class GroupService implements OnDestroy { public invite(playerId: number) { this.soundService.play(Sound.Ok); - this.clientService.createPartyInvite(playerId); + this.playerListService.sendPartyInvite(playerId); } async accept(inviterId: number) { @@ -279,7 +272,7 @@ export class GroupService implements OnDestroy { this.soundService.play(Sound.Ok); - this.clientService.acceptPartyInvite(inviterId); + this.playerListService.acceptPartyInvite(inviterId); } } diff --git a/Code/skyrim_ui/src/app/services/player-list.service.ts b/Code/skyrim_ui/src/app/services/player-list.service.ts index cc81ac68e..bd8fbeb56 100644 --- a/Code/skyrim_ui/src/app/services/player-list.service.ts +++ b/Code/skyrim_ui/src/app/services/player-list.service.ts @@ -1,6 +1,7 @@ import { Injectable, OnDestroy } from '@angular/core'; import { TranslocoService } from '@ngneat/transloco'; import { BehaviorSubject, Subscription } from 'rxjs'; +import { PartyInvite } from '../models/party-invite'; import { Player } from '../models/player'; import { PlayerList } from '../models/player-list'; import { PlayerManagerTab } from '../models/player-manager-tab.enum'; @@ -22,6 +23,17 @@ export class PlayerListService implements OnDestroy { private memberKickedSubscription: Subscription; private cellSubscription: Subscription; private partyInviteReceivedSubscription: Subscription; + private partyInfoSubscription: Subscription; + private partyLeftSubscription: Subscription; + + private readonly inviteCooldownMs = 3000; + private readonly sentInviteTimers = new Map(); + private readonly receivedInvites = new Map< + number, + { expiresAt: number; timer: number } + >(); + private partyLeaderId: number | undefined; + private partyMemberIds: number[] = []; private isConnect = false; @@ -38,15 +50,21 @@ export class PlayerListService implements OnDestroy { this.onMemberKicked(); this.onCellChange(); this.onPartyInviteReceived(); + this.onPartyInfo(); + this.onPartyLeft(); } ngOnDestroy() { + this.resetPartyInvitations(); this.debugSubscription.unsubscribe(); this.connectionSubscription.unsubscribe(); this.playerConnectedSubscription.unsubscribe(); this.playerDisconnectedSubscription.unsubscribe(); + this.memberKickedSubscription.unsubscribe(); this.cellSubscription.unsubscribe(); this.partyInviteReceivedSubscription.unsubscribe(); + this.partyInfoSubscription.unsubscribe(); + this.partyLeftSubscription.unsubscribe(); } private onDebug() { @@ -62,6 +80,7 @@ export class PlayerListService implements OnDestroy { return; } this.isConnect = connect; + this.resetPartyState(); this.playerList.next(undefined); this.updatePlayerList(); @@ -85,6 +104,8 @@ export class PlayerListService implements OnDestroy { this.playerDisconnectedSubscription = this.clientService.playerDisconnectedChange.subscribe( (playerDisco: Player) => { + this.clearSentInvite(playerDisco.id); + this.clearReceivedInvite(playerDisco.id); const playerList = this.getPlayerList(); if (playerList) { @@ -127,22 +148,69 @@ export class PlayerListService implements OnDestroy { private onPartyInviteReceived() { this.partyInviteReceivedSubscription = this.clientService.partyInviteReceivedChange.subscribe( - async (inviterId: number) => { - const playerList = this.getPlayerList(); + ({ inviterId, expiresInMs }: PartyInvite) => { + const invitingPlayer = this.getPlayerById(inviterId); + if ( + !this.isConnect || + this.partyLeaderId !== undefined || + !invitingPlayer || + inviterId === this.clientService.localPlayerId + ) { + return; + } - if (playerList) { - const invitingPlayer = this.getPlayerById(inviterId); - invitingPlayer.hasInvitedLocalPlayer = true; - this.playerList.next(playerList); - this.popupNotificationService.addPartyInvite( - invitingPlayer.name, - () => this.acceptPartyInvite(inviterId), - ); + this.clearReceivedInvite(inviterId); + if (!Number.isFinite(expiresInMs) || expiresInMs <= 0) { + this.updatePlayerList(); + return; } + + const invitation = { + expiresAt: performance.now() + expiresInMs, + timer: setTimeout(() => { + this.clearReceivedInvite(inviterId); + this.updatePlayerList(); + }, expiresInMs), + }; + this.receivedInvites.set(inviterId, invitation); + invitingPlayer.hasInvitedLocalPlayer = true; + this.updatePlayerList(); + this.popupNotificationService.addPartyInvite( + invitingPlayer.name, + () => { + // A popup for a replaced invite must not accept the newer one. + if (this.receivedInvites.get(inviterId) === invitation) { + this.acceptPartyInvite(inviterId); + } + }, + ); }, ); } + private onPartyInfo() { + this.partyInfoSubscription = this.clientService.partyInfoChange.subscribe( + partyInfo => { + if (this.partyLeaderId !== partyInfo.leaderId) { + this.resetHasBeenInvitedFlags(); + } + this.partyLeaderId = partyInfo.leaderId; + this.partyMemberIds = partyInfo.playerIds; + this.clearReceivedInvites(); + for (const playerId of partyInfo.playerIds) { + this.clearSentInvite(playerId); + } + this.updatePlayerList(); + }, + ); + } + + private onPartyLeft() { + this.partyLeftSubscription = this.clientService.partyLeftChange.subscribe( + () => this.resetPartyState(), + ); + } + public getLocalPlayer(): Player { let localPlayerId = this.clientService.localPlayerId; return this.getPlayerById(localPlayerId); @@ -169,27 +237,51 @@ export class PlayerListService implements OnDestroy { } public sendPartyInvite(inviteeId: number) { - const playerList = this.getPlayerList(); - - if (playerList) { - this.getPlayerById(inviteeId).hasBeenInvited = true; - - this.updatePlayerList(); - - this.clientService.createPartyInvite(inviteeId); + const player = this.getPlayerById(inviteeId); + if ( + !this.isConnect || + !player || + this.partyLeaderId !== this.clientService.localPlayerId || + inviteeId === this.clientService.localPlayerId || + this.partyMemberIds.includes(inviteeId) || + this.sentInviteTimers.has(inviteeId) + ) { + return; } + + player.hasBeenInvited = true; + this.sentInviteTimers.set( + inviteeId, + setTimeout(() => { + this.clearSentInvite(inviteeId); + this.updatePlayerList(); + }, this.inviteCooldownMs), + ); + this.updatePlayerList(); + this.clientService.createPartyInvite(inviteeId); } public acceptPartyInvite(inviterId: number) { - const playerList = this.getPlayerList(); - - if (playerList) { - this.getPlayerById(inviterId).hasInvitedLocalPlayer = false; - - this.clientService.acceptPartyInvite(inviterId); - - this.playerList.next(playerList); + const invitation = this.receivedInvites.get(inviterId); + if ( + !this.isConnect || + this.partyLeaderId !== undefined || + !this.getPlayerById(inviterId) || + !invitation + ) { + return; } + if (performance.now() >= invitation.expiresAt) { + this.clearReceivedInvite(inviterId); + this.updatePlayerList(); + return; + } + + // The server can reject this invite. Keep other invitations until partyInfo + // confirms a successful join, and consume this one before the bridge call. + this.clearReceivedInvite(inviterId); + this.updatePlayerList(); + this.clientService.acceptPartyInvite(inviterId); } public getPlayerById(playerId: number): Player { @@ -197,7 +289,11 @@ export class PlayerListService implements OnDestroy { } public resetHasBeenInvitedFlags() { - const playerList = this.getPlayerList(); + for (const timer of this.sentInviteTimers.values()) { + clearTimeout(timer); + } + this.sentInviteTimers.clear(); + const playerList = this.playerList.getValue(); if (playerList) { for (const player of playerList.players) { @@ -207,4 +303,44 @@ export class PlayerListService implements OnDestroy { this.updatePlayerList(); } } + + public resetPartyInvitations() { + this.resetHasBeenInvitedFlags(); + this.clearReceivedInvites(); + this.updatePlayerList(); + } + + public resetPartyState() { + this.partyLeaderId = undefined; + this.partyMemberIds = []; + this.resetPartyInvitations(); + } + + private clearSentInvite(playerId: number) { + clearTimeout(this.sentInviteTimers.get(playerId)); + this.sentInviteTimers.delete(playerId); + const player = this.playerList + .getValue() + ?.players.find(player => player.id === playerId); + if (player) { + player.hasBeenInvited = false; + } + } + + private clearReceivedInvite(playerId: number) { + clearTimeout(this.receivedInvites.get(playerId)?.timer); + this.receivedInvites.delete(playerId); + const player = this.playerList + .getValue() + ?.players.find(player => player.id === playerId); + if (player) { + player.hasInvitedLocalPlayer = false; + } + } + + private clearReceivedInvites() { + for (const playerId of this.receivedInvites.keys()) { + this.clearReceivedInvite(playerId); + } + } } diff --git a/Code/skyrim_ui/src/typings.d.ts b/Code/skyrim_ui/src/typings.d.ts index 0104f5d0c..682846fa4 100644 --- a/Code/skyrim_ui/src/typings.d.ts +++ b/Code/skyrim_ui/src/typings.d.ts @@ -93,7 +93,10 @@ declare namespace SkyrimTogetherTypes { type PartyLeftCallback = (inviterId: number) => void; - type PartyInviteReceivedCallback = (inviterId: number) => void; + type PartyInviteReceivedCallback = ( + inviterId: number, + expiresInMs?: number, + ) => void; } /** Global Skyrim: Together object. */ diff --git a/Code/tests/server/party_invitations.cpp b/Code/tests/server/party_invitations.cpp new file mode 100644 index 000000000..b3606fec5 --- /dev/null +++ b/Code/tests/server/party_invitations.cpp @@ -0,0 +1,62 @@ +#include + +#include + +TEST_CASE("Party invitation acceptance enforces its deadline without a cleanup pass", "[server.party]") +{ + PartyComponent party; + party.Invitations[1] = 60000; + party.Invitations[2] = 60000; + party.Invitations[3] = 60000; + + REQUIRE(party.TryConsumeInvitation(1, 59999)); + REQUIRE_FALSE(party.TryConsumeInvitation(1, 59999)); + REQUIRE_FALSE(party.TryConsumeInvitation(2, 60000)); + REQUIRE_FALSE(party.TryConsumeInvitation(3, 60001)); + REQUIRE(party.Invitations.empty()); +} + +TEST_CASE("Accepting an invitation cannot consume another player's invitation", "[server.party]") +{ + PartyComponent party; + party.Invitations[10] = 60000; + party.Invitations[20] = 61000; + + REQUIRE_FALSE(party.TryConsumeInvitation(30, 1000)); + REQUIRE(party.Invitations.size() == 2); + REQUIRE_FALSE(party.TryConsumeInvitation(10, 60000)); + REQUIRE(party.TryConsumeInvitation(20, 60000)); + REQUIRE(party.Invitations.empty()); +} + +TEST_CASE("Resending an invitation renews the deadline used by cleanup and acceptance", "[server.party]") +{ + PartyComponent party; + party.Invitations[10] = 60000; + party.Invitations[20] = 60000; + party.Invitations[10] = 90000; + + party.ExpireInvitations(60000); + + REQUIRE(party.Invitations.size() == 1); + REQUIRE_FALSE(party.TryConsumeInvitation(20, 60000)); + REQUIRE(party.TryConsumeInvitation(10, 89999)); + REQUIRE(party.Invitations.empty()); +} + +TEST_CASE("Party invitation cleanup preserves live invitations and removes expired ones", "[server.party]") +{ + PartyComponent party; + for (uint32_t inviter = 0; inviter < 100; ++inviter) + party.Invitations[inviter] = 60000 + inviter; + + party.ExpireInvitations(60049); + REQUIRE(party.Invitations.size() == 50); + for (uint32_t inviter = 0; inviter < 100; ++inviter) + REQUIRE(party.Invitations.contains(inviter) == (inviter >= 50)); + + party.ExpireInvitations(60099); + REQUIRE(party.Invitations.empty()); + party.ExpireInvitations(60100); + REQUIRE(party.Invitations.empty()); +} diff --git a/Code/tests/server/xmake.lua b/Code/tests/server/xmake.lua new file mode 100644 index 000000000..4b177457a --- /dev/null +++ b/Code/tests/server/xmake.lua @@ -0,0 +1,17 @@ +target("TPServerTests") + set_kind("binary") + set_group("Tests") + add_includedirs("../../server", "../../../Libraries") + set_pcxxheader("../../server/Pch.h") + add_files("../main.cpp", "*.cpp") + add_deps("SkyrimEncoding", "CommonLib", "Console", "TiltedConnect") + add_packages( + "tiltedcore", + "hopscotch-map", + "catch2", + "glm", + "entt", + "spdlog", + "gamenetworkingsockets", + "lua", + "sol2") diff --git a/Code/tests/xmake.lua b/Code/tests/xmake.lua index 1ce224fa8..38dac9756 100644 --- a/Code/tests/xmake.lua +++ b/Code/tests/xmake.lua @@ -13,3 +13,13 @@ target("TPTests") "catch2", "mimalloc", "glm") + +option("server_tests") + set_default(false) + set_showmenu(true) + set_description("Build isolated server component tests") +option_end() + +if has_config("server_tests") then + includes("server") +end