Skip to content

Commit affabdc

Browse files
AurelioloAureliolo
committed
fix: address review findings from pre-PR agents
- Fix self-reassignment bug on team delete (reject reassign_to=self) - Add self-reassign test case - Make TeamConfig.lead required (matches backend TeamResponse) - Fix untrimmed member names in TeamEditDialog - Fix irreversible select in TeamDeleteConfirmDialog - Align dialog styling with established pattern (rounded-xl, bg-surface, p-card) - Remove dead departmentName prop from TeamListSection - Fix variable shadowing in PackSelectionDialog onApply - Use dedicated COMPANY_BUDGET_UNDER_ALLOCATED event constant - Add logger to rebalance.py module - Remove orphaned comment in teams.py Pre-reviewed by 3 agents, 10 findings addressed
1 parent 00ada2e commit affabdc

13 files changed

Lines changed: 43 additions & 24 deletions

File tree

‎src/synthorg/api/controllers/teams.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,9 @@ async def delete_team(
430430
team_idx, team = _find_team(teams, team_name)
431431

432432
if reassign_to is not None:
433+
if reassign_to.strip().casefold() == team_name.strip().casefold():
434+
msg = "Cannot reassign members to the team being deleted"
435+
raise ApiValidationError(msg)
433436
target_idx, target = _find_team(teams, reassign_to)
434437
# Merge members (deduplicate, case-insensitive).
435438
existing_members = list(target.get("members", []))
@@ -444,7 +447,6 @@ async def delete_team(
444447
teams[target_idx] = updated_target
445448

446449
teams.pop(team_idx)
447-
# Adjust target_idx if it was after the removed team.
448450
dept = {**dept, "teams": teams}
449451
depts[dept_idx] = dept
450452

‎src/synthorg/budget/rebalance.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99
from typing import Any, NamedTuple
1010

1111
from synthorg.constants import BUDGET_ROUNDING_PRECISION
12+
from synthorg.observability import get_logger
13+
14+
logger = get_logger(__name__)
1215

1316

1417
class RebalanceMode(StrEnum):

‎src/synthorg/core/company.py‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@
1111
from synthorg.core.enums import AutonomyLevel, CompanyType
1212
from synthorg.core.types import NotBlankStr # noqa: TC001
1313
from synthorg.observability import get_logger
14-
from synthorg.observability.events.company import COMPANY_VALIDATION_ERROR
14+
from synthorg.observability.events.company import (
15+
COMPANY_BUDGET_UNDER_ALLOCATED,
16+
COMPANY_VALIDATION_ERROR,
17+
)
1518
from synthorg.security.autonomy.models import AutonomyConfig
1619
from synthorg.security.timeout.config import (
1720
ApprovalTimeoutConfig,
@@ -641,10 +644,8 @@ def _validate_departments(self) -> Self:
641644
raise ValueError(msg)
642645
if total > 0 and round(total, BUDGET_ROUNDING_PRECISION) < max_budget_percent:
643646
logger.info(
644-
COMPANY_VALIDATION_ERROR,
645-
warning=(
646-
f"Department budgets sum to {total:.2f}%, "
647-
f"under {max_budget_percent:.0f}%"
648-
),
647+
COMPANY_BUDGET_UNDER_ALLOCATED,
648+
total_percent=round(total, BUDGET_ROUNDING_PRECISION),
649+
max_percent=max_budget_percent,
649650
)
650651
return self

‎src/synthorg/observability/events/company.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@
33
from typing import Final
44

55
COMPANY_VALIDATION_ERROR: Final[str] = "company.validation.error"
6+
COMPANY_BUDGET_UNDER_ALLOCATED: Final[str] = "company.budget.under_allocated"

‎tests/unit/api/controllers/test_teams.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,19 @@ def test_delete_team_not_found(
287287
)
288288
assert resp.status_code == 404
289289

290+
def test_delete_team_self_reassign_rejected(
291+
self,
292+
test_client: TestClient[Any],
293+
) -> None:
294+
_seed_departments(
295+
test_client,
296+
[_dept_with_teams(teams=[_team("backend", members=["alice"])])],
297+
)
298+
resp = test_client.delete(
299+
"/api/v1/departments/engineering/teams/backend?reassign_to=backend",
300+
)
301+
assert resp.status_code == 422
302+
290303
def test_delete_team_with_reassign(
291304
self,
292305
test_client: TestClient[Any],

‎web/src/__tests__/pages/org/build-org-tree.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ describe('buildOrgTree', () => {
299299
name: 'engineering',
300300
display_name: 'Engineering',
301301
teams: [
302-
{ name: 'backend', members: ['Lead', 'Senior', 'Junior'] },
302+
{ name: 'backend', lead: 'Lead', members: ['Lead', 'Senior', 'Junior'] },
303303
],
304304
},
305305
])

‎web/src/api/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,7 @@ export interface Department {
609609

610610
export interface TeamConfig {
611611
name: string
612-
lead?: string
612+
lead: string
613613
readonly members: readonly string[]
614614
}
615615

‎web/src/pages/org-edit/DepartmentEditDrawer.tsx‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,6 @@ export function DepartmentEditDrawer({
161161
)}
162162

163163
<TeamListSection
164-
departmentName={department.name}
165164
teams={department.teams}
166165
saving={saving}
167166
onCreateTeam={(data) => onCreateTeam(department.name, data)}

‎web/src/pages/org-edit/PackApplyPreviewDialog.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export function PackApplyPreviewDialog({
4949
<Dialog.Root open={open} onOpenChange={onOpenChange}>
5050
<Dialog.Portal>
5151
<Dialog.Backdrop className="fixed inset-0 z-50 bg-bg-base/80 backdrop-blur-sm transition-[opacity,translate] data-[closed]:opacity-0 data-[starting-style]:opacity-0" />
52-
<Dialog.Popup className="fixed top-1/2 left-1/2 z-50 w-full max-w-lg -translate-x-1/2 -translate-y-1/2 rounded-lg border border-border bg-bg-card p-6 shadow-xl transition-[opacity,translate] data-[closed]:scale-95 data-[closed]:opacity-0 data-[starting-style]:scale-95 data-[starting-style]:opacity-0">
52+
<Dialog.Popup className="fixed top-1/2 left-1/2 z-50 w-full max-w-lg -translate-x-1/2 -translate-y-1/2 rounded-xl border border-border-bright bg-surface p-card shadow-[var(--so-shadow-card-hover)] transition-[opacity,translate] data-[closed]:scale-95 data-[closed]:opacity-0 data-[starting-style]:scale-95 data-[starting-style]:opacity-0">
5353
<Dialog.Title className="text-base font-semibold text-text-primary">
5454
Apply {pack?.display_name ?? 'Pack'}
5555
</Dialog.Title>

‎web/src/pages/org-edit/PackSelectionDialog.tsx‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -246,10 +246,7 @@ export function PackSelectionDialog({ open, onOpenChange, disabled }: PackSelect
246246
disabled={disabled ?? false}
247247
busy={busy}
248248
applying={applying}
249-
onApply={(name) => {
250-
const pack = packs.find((p) => p.name === name)
251-
if (pack) handleSelectPack(pack)
252-
}}
249+
onApply={() => handleSelectPack(pack)}
253250
/>
254251
</StaggerItem>
255252
))}

0 commit comments

Comments
 (0)