Ревью кода (#82) #90

Merged
NotBigGhost merged 4 commits from issue-82-code-review into dev 2026-09-18 23:42:36 +03:00
7 changed files with 105 additions and 114 deletions
Showing only changes of commit 5035ee41e6 - Show all commits
+3 -4
View File
@@ -315,16 +315,15 @@ def delete_match(
session: Session = Depends(get_session),
admin: User = Depends(get_current_admin),
) -> s.OkResponse:
group_id = match_service.get_match(session, match_id).group_id # для уведомления
# До удаления: каскад унесёт участников вместе с партией.
participant_ids = notify.match_participant_ids(session, match_id)
match = match_service.get_match(session, match_id)
group_id, finished = match.group_id, match.status == "finished" # для уведомления
admin_service.delete_match(session, match_id)
audit_service.record(
session, actor_id=admin.id, action="delete", entity_type="match", entity_id=match_id,
ip=client_ip(request),
)
session.commit()
notify.match_removed(session, match_id, group_id, participant_ids)
notify.match_removed(session, match_id, group_id, finished=finished)
return s.OkResponse()
+2 -4
View File
@@ -352,9 +352,7 @@ def delete_match(
match_service.assert_can_modify(session, match, user)
match_id_val = match.id
group_id_val = match.group_id
# Участников читаем до удаления: каскад унесёт их строки вместе с партией,
# а событию они нужны, чтобы клиент знал, чьи витрины протухли.
participant_ids = notify.match_participant_ids(session, match_id_val) # type: ignore[arg-type]
finished = match.status == "finished" # после удаления статус уже не прочитать
match_service.delete_match(session, match, expected_version=expected_version)
audit_service.record(
session,
@@ -367,5 +365,5 @@ def delete_match(
user_agent=request.headers.get("user-agent"),
)
session.commit()
notify.match_removed(session, match_id_val, group_id_val, participant_ids) # type: ignore[arg-type]
notify.match_removed(session, match_id_val, group_id_val, finished=finished) # type: ignore[arg-type]
return s.OkResponse()
+26 -37
View File
@@ -8,7 +8,7 @@ from __future__ import annotations
from sqlmodel import Session, select
from app.core.events import hub
from app.models import GroupMember, Match, MatchParticipant, User
from app.models import GroupMember, Match, User
def _group_member_ids(session: Session, group_id: int) -> list[int]:
@@ -17,30 +17,28 @@ def _group_member_ids(session: Session, group_id: int) -> list[int]:
)
def match_participant_ids(session: Session, match_id: int) -> list[int]:
"""Кто играл в партии. Нужен в событии, чтобы клиент понимал, чьи витрины
(история игр, публичный профиль, личная статистика) реально протухли."""
return list(
session.exec(
select(MatchParticipant.user_id).where(MatchParticipant.match_id == match_id)
def _ratings_changed(session: Session, notified: list[int]) -> None:
"""Рейтинг общий и считается по всей истории (#80): завершённая партия двигает топ,
главную, историю и профили всех, кто играл после неё, и страницы других групп.
Игрокам вне группы (notified уже знают) — событие без подробностей о партии."""
skip = set(notified)
ids = [
uid
for uid in session.exec(
select(User.id).where(User.role == "player", User.is_active.is_(True)) # type: ignore[union-attr]
).all()
)
if uid not in skip
]
hub.publish(ids, {"type": "ratings"})
def match_changed(session: Session, match: Match) -> None:
"""Партия изменилась — уведомить всех участников её группы.
Адресат — вся группа: списки партий и статистика группы меняются у всех. А вот
история и профили протухают только у игравших, поэтому их id едут в событии."""
hub.publish(
_group_member_ids(session, match.group_id),
{
"type": "match",
"match_id": match.id,
"group_id": match.group_id,
"participant_ids": match_participant_ids(session, match.id), # type: ignore[arg-type]
},
)
"""Партия изменилась — уведомить всех участников её группы, а если она завершена —
и остальных игроков (_ratings_changed)."""
members = _group_member_ids(session, match.group_id)
hub.publish(members, {"type": "match", "match_id": match.id, "group_id": match.group_id})
if match.status == "finished":
_ratings_changed(session, members)
def match_draft_changed(session: Session, match: Match, actor_id: int) -> None:
@@ -53,22 +51,13 @@ def match_draft_changed(session: Session, match: Match, actor_id: int) -> None:
hub.publish(ids, {"type": "match_draft", "match_id": match.id, "group_id": match.group_id})
def match_removed(
session: Session, match_id: int, group_id: int, participant_ids: list[int] | None = None
) -> None:
"""Партия удалена — уведомить участников группы (обновить списки).
participant_ids передаются снаружи: к этому моменту партии уже нет, а её участники
ушли каскадом, и собрать их из базы невозможно."""
hub.publish(
_group_member_ids(session, group_id),
{
"type": "match",
"match_id": match_id,
"group_id": group_id,
"participant_ids": participant_ids or [],
},
)
def match_removed(session: Session, match_id: int, group_id: int, *, finished: bool) -> None:
"""Партия удалена — уведомить участников группы (обновить списки), а если она была
завершена — и остальных игроков. Статус передаётся снаружи: партии уже нет."""
members = _group_member_ids(session, group_id)
hub.publish(members, {"type": "match", "match_id": match_id, "group_id": group_id})
if finished:
_ratings_changed(session, members)
def group_changed(session: Session, group_id: int, extra_user_ids: list[int] | None = None) -> None:
+34 -28
View File
@@ -1,4 +1,5 @@
"""Событие партии несёт список участников: по нему клиент решает, чьи витрины протухли."""
"""Адресаты событий партии. Рейтинг общий (#80): завершённая партия двигает витрины
всех игроков, поэтому игроки вне группы получают событие ratings (#88)."""
from __future__ import annotations
from fastapi.testclient import TestClient
@@ -16,20 +17,29 @@ def _capture_events(monkeypatch) -> list[tuple[list[int], dict]]:
return published
def _match_events(published: list[tuple[list[int], dict]]) -> list[dict]:
return [e for _ids, e in published if e.get("type") == "match"]
def _recipients(published: list[tuple[list[int], dict]], kind: str) -> set[int]:
return {uid for ids, e in published if e.get("type") == kind for uid in ids}
def test_match_event_carries_participants(client: TestClient, engine, monkeypatch):
def _two_groups(client: TestClient, engine) -> tuple[dict, int, int, int, list[int]]:
"""Хост и Игрок2 в группе партии, Чужой — только в другой группе хоста."""
me = login(client, "Хост")
exps = [e["id"] for e in client.get("/api/expansions").json()]
gid = client.post(
"/api/groups", json={"name": "Группа", "expansion_ids": exps}, headers=csrf_headers(client)
def group(name: str) -> int:
return client.post(
"/api/groups", json={"name": name, "expansion_ids": exps}, headers=csrf_headers(client)
).json()["id"]
gid, other_gid = group("Группа"), group("Другая")
p2 = add_group_member(engine, gid, "Игрок2")
# Третий в группе, но НЕ в партии: его история от этой партии не меняется.
p3 = add_group_member(engine, gid, "Зритель")
outsider = add_group_member(engine, other_gid, "Чужой")
fids = [f["id"] for f in client.get(f"/api/groups/{gid}/factions").json()]
return me, gid, p2, outsider, fids
def test_finished_match_reaches_players_outside_group(client: TestClient, engine, monkeypatch):
me, gid, p2, outsider, fids = _two_groups(client, engine)
published = _capture_events(monkeypatch)
started = start_match(
@@ -37,36 +47,31 @@ def test_match_event_carries_participants(client: TestClient, engine, monkeypatc
[{"user_id": me["id"], "faction_id": fids[0]}, {"user_id": p2, "faction_id": fids[1]}],
)
assert started.status_code == 200, started.text
mid = started.json()["id"]
ev = _match_events(published)[-1]
assert sorted(ev["participant_ids"]) == sorted([me["id"], p2])
assert p3 not in ev["participant_ids"]
# Незавершённая партия рейтинг не двигает — знать о ней нужно только группе.
assert _recipients(published, "match") == {me["id"], p2}
assert _recipients(published, "ratings") == set()
published.clear()
fin = finish_match(
client, mid, [{"user_id": me["id"], "place": 1}, {"user_id": p2, "place": 2}]
client, started.json()["id"], [{"user_id": me["id"], "place": 1}, {"user_id": p2, "place": 2}]
)
assert fin.status_code == 200, fin.text
assert sorted(_match_events(published)[-1]["participant_ids"]) == sorted([me["id"], p2])
assert _recipients(published, "match") == {me["id"], p2}
ratings = _recipients(published, "ratings")
assert outsider in ratings
assert not ratings & {me["id"], p2} # группа уже получила подробное событие
def test_delete_event_carries_participants(client: TestClient, engine, monkeypatch):
"""Удаление — главный случай: строки участников уже уничтожены каскадом.
Если собирать их после удаления, список всегда окажется пустым, и клиент не
обновит историю тем, кто в этой партии играл."""
me = login(client, "Хост")
exps = [e["id"] for e in client.get("/api/expansions").json()]
gid = client.post(
"/api/groups", json={"name": "Группа", "expansion_ids": exps}, headers=csrf_headers(client)
).json()["id"]
p2 = add_group_member(engine, gid, "Игрок2")
fids = [f["id"] for f in client.get(f"/api/groups/{gid}/factions").json()]
def test_deleting_finished_match_reaches_players_outside_group(
client: TestClient, engine, monkeypatch
):
"""Удаление завершённой партии пересчитывает рейтинг всех, кто играл после неё."""
me, gid, p2, outsider, fids = _two_groups(client, engine)
mid = start_match(
client, gid,
[{"user_id": me["id"], "faction_id": fids[0]}, {"user_id": p2, "faction_id": fids[1]}],
).json()["id"]
finish_match(client, mid, [{"user_id": me["id"], "place": 1}, {"user_id": p2, "place": 2}])
published = _capture_events(monkeypatch)
version = client.get(f"/api/matches/{mid}").json()["version"]
@@ -74,4 +79,5 @@ def test_delete_event_carries_participants(client: TestClient, engine, monkeypat
f"/api/matches/{mid}", params={"expected_version": version}, headers=csrf_headers(client)
)
assert r.status_code == 200, r.text
assert sorted(_match_events(published)[-1]["participant_ids"]) == sorted([me["id"], p2])
assert _recipients(published, "match") == {me["id"], p2}
assert outsider in _recipients(published, "ratings")
+19 -4
View File
@@ -1,3 +1,5 @@
import type { QueryClient } from "@tanstack/react-query";
export const qk = {
me: ["me"] as const,
adminMe: ["adminMe"] as const,
@@ -30,10 +32,11 @@ export const qk = {
};
/**
* Ключи, которые протухают от любой партии: конкретных участников мы не знаем
* (событие приходит на всю группу), поэтому инвалидируем по префиксу. Один
* список на SSE-обработчик и на завершение партии — иначе переименование ключа
* в этом файле тихо разойдётся с местами, где он написан строкой.
* Ключи, которые протухают от любой завершённой партии: рейтинг общий и считается по
* всей истории (#80), так что партия двигает топ, историю и профили всех, кто играл
* после неё. Поэтому инвалидируем по префиксу. Один список на SSE-обработчик и на
* мутации партии — иначе переименование ключа тихо разойдётся с местами, где он
* написан строкой.
*/
export const matchAffectedKeys = [
qk.home,
@@ -42,3 +45,15 @@ export const matchAffectedKeys = [
["userMatches"],
["publicProfile"],
] as const;
/**
* Все рейтинговые витрины, включая статистику любой группы: рейтинг игроков в ней
* общий, так что его двигает и партия другой группы (#88). Перезапрашиваются только
* открытые на экране запросы, остальные лишь помечаются устаревшими.
*/
export function invalidateRatingViews(qc: QueryClient) {
for (const key of matchAffectedKeys) qc.invalidateQueries({ queryKey: key });
qc.invalidateQueries({
predicate: (q) => q.queryKey[0] === "group" && q.queryKey[2] === "stats",
});
}
+4 -6
View File
@@ -1,7 +1,7 @@
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { api, unwrap } from "../api/client";
import { matchAffectedKeys, qk } from "../api/queryKeys";
import { invalidateRatingViews, qk } from "../api/queryKeys";
import type {
FactionRead,
MatchCreate,
@@ -83,8 +83,7 @@ export function useFinishMatch() {
onSuccess: (m) => {
qc.invalidateQueries({ queryKey: qk.match(m.id) });
qc.invalidateQueries({ queryKey: qk.groupMatches(m.group_id) });
qc.invalidateQueries({ queryKey: qk.groupStats(m.group_id) });
for (const key of matchAffectedKeys) qc.invalidateQueries({ queryKey: key });
invalidateRatingViews(qc);
},
});
}
@@ -103,9 +102,8 @@ export function useUpdateMatch() {
onSuccess: (m) => {
qc.setQueryData(qk.match(m.id), m);
qc.invalidateQueries({ queryKey: qk.groupMatches(m.group_id) });
qc.invalidateQueries({ queryKey: qk.groupStats(m.group_id) });
// Места изменились — значит изменились лидерборд, история и профили.
for (const key of matchAffectedKeys) qc.invalidateQueries({ queryKey: key });
// Места изменились — значит изменились рейтинги, топ, истории и профили.
invalidateRatingViews(qc);
},
});
}
+16 -30
View File
@@ -1,15 +1,20 @@
import { useQueryClient } from "@tanstack/react-query";
import { useEffect, useRef } from "react";
import { useEffect } from "react";
import { matchAffectedKeys, qk } from "../api/queryKeys";
import { useMe } from "./auth";
import { invalidateRatingViews, qk } from "../api/queryKeys";
interface ServerEvent {
type: "match" | "match_draft" | "group" | "invitations" | "notifications" | "announcements";
/** ratings — завершённая партия чужой группы сдвинула общий рейтинг (#88). */
type:
| "match"
| "match_draft"
| "ratings"
| "group"
| "invitations"
| "notifications"
| "announcements";
match_id?: number;
group_id?: number;
/** Кто играл в партии: их история и профили протухли, чужие — нет. */
participant_ids?: number[];
}
/**
@@ -19,11 +24,6 @@ interface ServerEvent {
*/
export function useServerEvents(enabled: boolean) {
const qc = useQueryClient();
const { data: me } = useMe();
// Свой id — в ref: положив его в зависимости эффекта, мы бы пересоздавали
// SSE-соединение каждый раз, когда профиль перезапрашивается.
const myId = useRef<number | null>(null);
myId.current = me?.id ?? null;
useEffect(() => {
if (!enabled) return;
const base = import.meta.env.VITE_API_BASE_URL || "";
@@ -50,26 +50,12 @@ export function useServerEvents(enabled: boolean) {
if (ev.match_id != null) qc.invalidateQueries({ queryKey: qk.match(ev.match_id) });
if (ev.group_id != null) {
qc.invalidateQueries({ queryKey: qk.groupMatches(ev.group_id) });
qc.invalidateQueries({ queryKey: qk.groupStats(ev.group_id) });
}
// Общее меняется от любой партии: рейтинг глобальный, и чужая игра двигает топ.
qc.invalidateQueries({ queryKey: qk.home });
qc.invalidateQueries({ queryKey: qk.leaderboard });
if (ev.participant_ids) {
// Личные витрины — только у игравших: иначе каждая партия в группе
// заставляла бы всех остальных перезапрашивать свою историю.
for (const pid of ev.participant_ids) {
qc.invalidateQueries({ queryKey: qk.userMatches(pid) });
qc.invalidateQueries({ queryKey: qk.publicProfile(pid) });
}
if (myId.current != null && ev.participant_ids.includes(myId.current)) {
qc.invalidateQueries({ queryKey: qk.myStats });
}
} else {
// Событие от бэкенда без списка участников (вкладка открыта до обновления
// сервера) — ведём себя как раньше, широко.
for (const key of matchAffectedKeys) qc.invalidateQueries({ queryKey: key });
}
// Рейтинг общий и считается по всей истории: партия двигает топ, историю
// и профили всех, кто играл после неё, и страницы других групп.
invalidateRatingViews(qc);
} else if (ev.type === "ratings") {
invalidateRatingViews(qc);
} else if (ev.type === "group") {
if (ev.group_id != null) {
qc.invalidateQueries({ queryKey: qk.group(ev.group_id) });