528 lines
46 KiB
Markdown
528 lines
46 KiB
Markdown
# Reviewer — soi đối-kháng diff ĐỢT 1 (S155, PE delete-by-approver) — cửa cuối trước DEPLOY PROD
|
|
|
|
- Ngày: 2026-07-27
|
|
- Vật soi: `git status` + `git diff` toàn repo tại HEAD `8d4075a` (working tree, chưa commit)
|
|
- Phạm vi: 11 file MODIFIED + 3 file UNTRACKED (2 migration + Designer, 1 test file mới) + 1 sub-report
|
|
- Lead đã tự đo: `dotnet build` 0W/0E · `dotnet test` 549 PASS / 0 FAIL. Reviewer KHÔNG chạy lại build/test (tin số đo của lead), nhưng đọc lại assert của test mới để tìm xanh-giả.
|
|
|
|
> Ghi TRONG LÚC LÀM. Mỗi mục ghi ngay khi tìm ra.
|
|
|
|
---
|
|
|
|
## BẢNG DIFF (đo bằng `git diff --stat` + `git status --porcelain`)
|
|
|
|
| File | Δ |
|
|
|---|---|
|
|
| `src/Backend/SolutionErp.Application/ApprovalWorkflowsV2/ApprovalWorkflowV2AdminFeatures.cs` | +605 |
|
|
| `tests/SolutionErp.Infrastructure.Tests/Api/AuthorizePolicyRegressionTests.cs` | +54 |
|
|
| `src/Backend/SolutionErp.Domain/ApprovalWorkflowsV2/ApprovalWorkflow.cs` | +31/-2 |
|
|
| `fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx` | +28 |
|
|
| `src/Backend/SolutionErp.Api/Controllers/ApprovalWorkflowsV2Controller.cs` | +17 |
|
|
| `src/Backend/SolutionErp.Infrastructure/Services/PurchaseEvaluationWorkflowService.cs` | +13/-1 |
|
|
| `.../Configurations/ApprovalWorkflowConfiguration.cs` | +6 |
|
|
| `fe-user/src/types/approvalWorkflowV2.ts` | +7/-1 |
|
|
| `.../ApplicationDbContextModelSnapshot.cs` | +5 |
|
|
| `fe-user/src/pages/pe/WorkflowMatrixViewPage.tsx` | +2 |
|
|
| UNTRACKED: `Migrations/20260727033522_AddPeAllowApproverDelete.{cs,Designer.cs}` | mới |
|
|
| UNTRACKED: `tests/.../Application/UpdateAwDefinitionTests.cs` | mới |
|
|
|
|
---
|
|
|
|
# FLAG
|
|
|
|
## 🔴 F1 — HIGH — `PurgeOrphanOpinionsAsync` (ExecuteDelete) chạy NGOÀI transaction và TRƯỚC `SaveChangesAsync` ⇒ mất dữ liệu không hoàn tác được
|
|
|
|
**File:** `src/Backend/SolutionErp.Application/ApprovalWorkflowsV2/ApprovalWorkflowV2AdminFeatures.cs:886-918` (thân `PurgeOrphanOpinionsAsync`), gọi tại **`:681-683`**, còn `SaveChangesAsync` mãi tận **`:822`**.
|
|
|
|
**Điều sai.** Handler này KHÔNG mở transaction (`grep -n "BeginTransaction" ApprovalWorkflowV2AdminFeatures.cs` → **0 hit** trong toàn file). `ExecuteDeleteAsync` phát `DELETE` thẳng xuống SQL và **auto-commit ngay** khi không có transaction ambient. Trình tự thực tế:
|
|
|
|
1. `PurgeOrphanOpinionsAsync` chạy **7 câu DELETE**, mỗi câu commit riêng → chữ ký đã BIẾN MẤT VĨNH VIỄN khỏi DB.
|
|
2. Sau đó mới tới `db.ApprovalWorkflowLevels.Remove(...)` (`:767-772`), truy vấn phiếu bị ảnh hưởng (`:794-800`), `db.PurchaseEvaluationChangelogs.Add(...)` (`:806`), rồi `SaveChangesAsync` (`:822`).
|
|
3. Nếu bước 3 ném (FK 547 từ một bảng chữ ký **thứ 8** chưa được liệt kê, unique index, `DbUpdateConcurrencyException`, timeout, mất kết nối, `OperationCanceledException` do client huỷ request), thì toàn bộ thay đổi ở bước 2 **rollback**, nhưng chữ ký ở bước 1 **không quay lại được**.
|
|
|
|
⇒ Kết quả: admin bấm Sửa, nhận **409/500**, tưởng "không có gì xảy ra", trong khi N dòng chữ ký đã bị xoá cứng. Đây đúng là class lỗi *sai IM LẶNG* mà chính spec S155 đang đi chữa.
|
|
|
|
Rủi ro này **không lý thuyết**: chính `PurgeOrphanOpinionsAsync` liệt kê tay 7 bảng (`:889`, `:893`, `:897`, `:901`, `:905`, `:909`, `:913`). Nếu tương lai có bảng `*LevelOpinion` thứ 8 (module Văn phòng số đang mở rộng), thì `CountLiveOpinionsAsync:830` bỏ sót → guard cho qua → purge 7 bảng → `Remove` → `SaveChanges` **nổ FK 547 ở bảng thứ 8** → rollback → mất chữ ký ở 7 bảng kia. Nghĩa là đường "purge thành công rồi SaveChanges fail" tồn tại thật, không phải giả định.
|
|
|
|
**Cách phá tao đã dựng (không chỉ đọc).** Đếm số nguồn ném NẰM GIỮA purge (`:683`) và SaveChanges (`:822`): `Remove()` ở `:771` có thể ném `InvalidOperationException` (severed association) nếu `DetachTrackedOpinionsOn` bắt hụt (xem F5); `db.PurchaseEvaluations…ToListAsync(ct)` ở `:794-800` là một **round-trip DB nữa** sau khi đã purge, huỷ token là ném; `ResolveUserNamesAsync` ở `:686` cũng là round-trip DB sau purge; và `SaveChanges` còn ghi changelog cho N phiếu. Bốn nguồn ném độc lập, đều nằm SAU điểm không-hoàn-tác.
|
|
|
|
**Đề xuất sửa.** Bọc toàn handler trong một transaction rõ ràng (`db.Database.BeginTransactionAsync(ct)` → purge → SaveChanges → `CommitAsync`), **hoặc** dời `PurgeOrphanOpinionsAsync` xuống SAU `SaveChangesAsync` (khi đó phải chấp nhận `Remove` nổ FK trước, nên phương án transaction sạch hơn). Nếu `IApplicationDbContext` không expose `Database`, cách rẻ nhất là chuyển thẳng handler sang nhận `ApplicationDbContext` hoặc thêm `IDbTransactionScope` — repo đã có tiền lệ transaction ở `ContractCodeGenerator.GenerateAsync` (`IsolationLevel.Serializable`).
|
|
|
|
**Tiêu chí nghiệm thu.** Có test: giả lập `SaveChanges` ném sau khi purge (ví dụ chèn một opinion mồ côi + ép lỗi ở changelog) rồi assert **số row opinion mồ côi KHÔNG đổi**. Test hiện tại không có ca này.
|
|
|
|
---
|
|
|
|
## 🔴 F2 — HIGH — Purge xoá CỨNG cả chữ ký của phiếu chỉ mới XOÁ MỀM ⇒ phá thẳng tính năng "khôi phục phiếu đã xoá" của chính đợt 2
|
|
|
|
**File:** `ApprovalWorkflowV2AdminFeatures.cs:889-891` (và 6 khối lặp y hệt tới `:915`), điều kiện:
|
|
|
|
```csharp
|
|
.Where(o => levelIds.Contains(o.ApprovalWorkflowLevelId)
|
|
&& (o.IsDeleted || !db.PurchaseEvaluations.Any(p => p.Id == o.PurchaseEvaluationId && !p.IsDeleted)))
|
|
.ExecuteDeleteAsync(ct);
|
|
```
|
|
|
|
**Điều sai — hai vế đều rộng hơn nhãn "mồ côi":**
|
|
|
|
- **Vế `!db.PurchaseEvaluations.Any(... && !p.IsDeleted)`**: phiếu **xoá mềm** (`IsDeleted=1`, dữ liệu còn nguyên, khôi phục được) bị coi ngang với phiếu không tồn tại. Chữ ký của nó bị **xoá CỨNG**. Nhưng chính spec S155 hạng mục A là "màn hình phiếu Đã xoá + khôi phục". Sau khi admin sửa quy trình một lần, phiếu xoá mềm khôi phục lên sẽ **mất trắng lịch sử ý kiến cấp duyệt** ở những Cấp bị gỡ người — im lặng, không cảnh báo.
|
|
- **Vế `o.IsDeleted`**: opinion **tự nó** xoá mềm trên một phiếu **CÒN SỐNG** cũng bị xoá cứng. Đây là bản ghi kiểm toán của tài liệu đang hiệu lực.
|
|
|
|
Cả 7 bảng đều dính (`ContractLevelOpinions:893`, `ProposalLevelOpinions:897`, `LeaveRequestLevelOpinions:901`, `OtRequestLevelOpinions:905`, `TravelRequestLevelOpinions:909`, `VehicleBookingLevelOpinions:913`).
|
|
|
|
**Cách phá.** Tao đọc ngược: có gì chặn không? `CountLiveOpinionsAsync:830-884` chỉ đếm opinion **còn sống**, nên đúng những opinion sắp bị xoá cứng là những opinion mà guard **cố tình bỏ qua** — tức guard và purge được thiết kế bù nhau, không có lớp nào canh phần "xoá mềm còn khôi phục được". Không có `changes.Add` nào ghi ID phiếu bị ảnh hưởng, chỉ có `:775` ghi số đếm `"Dọn {purgedOrphans} chữ ký mồ côi"` — không truy ngược được đã xoá của phiếu nào.
|
|
|
|
**Đề xuất sửa.** Hai lựa chọn, chọn 1:
|
|
- (a) **Thu hẹp**: chỉ purge khi phiếu cha **thật sự không còn row** (`!db.PurchaseEvaluations.IgnoreQueryFilters().Any(p => p.Id == o.PurchaseEvaluationId)`), còn phiếu xoá mềm thì **chặn admin** như phiếu sống (thông báo "phiếu X đã xoá đang giữ chữ ký ở Cấp này, khôi phục hoặc xoá hẳn trước").
|
|
- (b) **Chuyển FK sang nullable + SET NULL** để gỡ người không cần đụng chữ ký. (Đổi schema, không hợp đợt 1.)
|
|
|
|
Với đợt 1 sắp deploy, (a) là đường ít rủi ro nhất. Nếu owner chấp nhận giữ nguyên, tối thiểu phải ghi ID phiếu bị purge vào `changes` để còn dấu vết.
|
|
|
|
**Tiêu chí nghiệm thu.** Test: phiếu xoá mềm có 1 opinion ở Cấp bị gỡ → gọi Update → assert opinion **vẫn còn** (hoặc lệnh bị 409). Test hiện tại không có ca này.
|
|
|
|
---
|
|
|
|
## 🔴 F3 — HIGH — Endpoint PUT KHÔNG có dây FE nào gọi tới ⇒ mục tiêu số 1 của đợt 1 (cứu phiếu đang treo) KHÔNG dùng được trong UAT
|
|
|
|
**File BE:** `src/Backend/SolutionErp.Api/Controllers/ApprovalWorkflowsV2Controller.cs:45-51`
|
|
**File FE (nơi lẽ ra phải có):** `fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx:589` — submit của Designer.
|
|
|
|
**Bằng chứng đo được (không phải đọc thấy):**
|
|
|
|
```
|
|
$ grep -rn "approval-workflows-v2" fe-admin/src fe-user/src
|
|
… 29 hit, TOÀN BỘ là api.get / route / queryKey, cộng đúng 3 hit ghi:
|
|
fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx:286 api.delete(`/approval-workflows-v2/${id}`)
|
|
fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx:297 api.patch(`/approval-workflows-v2/${id}/user-selectable`, …)
|
|
fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx:589 api.post('/approval-workflows-v2', …)
|
|
|
|
$ grep -rn "api.put" fe-admin/src/pages/system/ApprovalWorkflowsV2Page.tsx
|
|
(0 hit)
|
|
```
|
|
|
|
**Điều sai.** ~600 dòng BE mới (`UpdateAwDefinitionCommand` + Validator + Handler + 3 helper) là **mã chết** đối với người dùng UAT. Designer trên UI chỉ có đường **POST tạo version mới**. Mà chính comment `:386-396` của file Application đã tự khai: POST sinh `Id` MỚI ⇒ phiếu đang treo pin `Id` CŨ ⇒ **cờ F6 vừa tick không bao giờ tới được phiếu cần cứu**.
|
|
|
|
⇒ Kịch bản UAT thật của owner: mở Designer → tick "Cho phép NV này XÓA phiếu" → Lưu → FE gọi POST → sinh version mới → phiếu bấm sai gói thầu **vẫn không xoá được**. Owner sẽ báo "làm rồi mà không ăn", đúng cái vòng lặp đợt 1 định chấm dứt.
|
|
|
|
Đây không phải lỗi mã — mã BE đúng. Đây là **lỗ hổng phạm vi**: nếu đợt 1 deploy để owner UAT, thì phần owner chạm được (FE) không nối vào phần vừa xây (BE).
|
|
|
|
**Đề xuất sửa.** Chọn 1 trong 3, khai rõ với owner trước khi deploy:
|
|
- (a) Nối dây FE trong đợt 1: Designer khi mở từ một version **đang tồn tại** thì `api.put('/approval-workflows-v2/'+id, payload)` thay vì POST, kèm nút phân biệt "Sửa tại chỗ" ⟂ "Tạo phiên bản mới".
|
|
- (b) Deploy đợt 1 nhưng **nói rõ** với owner rằng F6 chỉ áp cho phiếu tạo MỚI sau khi tick, còn phiếu đang treo phải chờ đợt 2 (kèm cách chữa tạm bằng `sqlcmd`/Swagger).
|
|
- (c) Hoãn phần PUT khỏi đợt 1.
|
|
|
|
**Tiêu chí nghiệm thu cho (a).** Sau deploy: tick cờ F6 cho một NV trên version đang có phiếu treo → `GET /api/approval-workflows-v2` trả `allowApproverDelete=true` **trên đúng `id` cũ** (Version không tăng), và phiếu treo đọc thấy cờ.
|
|
|
|
---
|
|
|
|
## 🟠 F4 — MED — 7 cờ Level bị GHI TUYỆT ĐỐI từ payload ⇒ client gửi thiếu field = **tắt cờ IM LẶNG**
|
|
|
|
**File:** `ApprovalWorkflowV2AdminFeatures.cs:424-440` (khai `UpdateAwLevelInput` với default) + **`:720-726`** (gán tuyệt đối).
|
|
|
|
```csharp
|
|
dbLevel.AllowReturnOneLevel = reqLevel.AllowReturnOneLevel; // default false
|
|
dbLevel.AllowReturnOneStep = reqLevel.AllowReturnOneStep; // default false
|
|
dbLevel.AllowReturnToAssignee = reqLevel.AllowReturnToAssignee; // default false
|
|
dbLevel.AllowReturnToDrafter = reqLevel.AllowReturnToDrafter; // default TRUE ← lệch chiều
|
|
dbLevel.AllowApproverEditDetails = reqLevel.AllowApproverEditDetails;
|
|
dbLevel.AllowApproverEditBudget = reqLevel.AllowApproverEditBudget;
|
|
dbLevel.AllowApproverDelete = reqLevel.AllowApproverDelete;
|
|
```
|
|
|
|
**Điều sai.** 7 cờ này là `bool` (không phải `bool?`) với default trong primary constructor. .NET 10 + System.Text.Json **có** tôn trọng default parameter của record, nên field vắng trong JSON → nhận default, **không** giữ giá trị đang lưu. Hai hệ quả:
|
|
|
|
- Client gửi payload thiếu `allowApproverEditBudget` → cờ đang BẬT bị **tắt**, trả 204, không ai biết.
|
|
- `AllowReturnToDrafter` default **`true`** trong khi 6 cờ kia default `false` — client gửi thiếu thì cờ này bị **BẬT** lên. Chiều lỗi ngược với 6 cờ còn lại, dễ bỏ sót nhất.
|
|
|
|
Đối chiếu: 2 cờ đổi-đường-đi (`AllowApproverSkipToFinal`, `AllowApproverFinalize`) được khai `bool?` với `null = không khai báo` (`:438-439`) và xử lý đúng kiểu echo — tức tác giả **đã biết** kỹ thuật này, chỉ không áp cho 7 cờ kia. Bất đối xứng nội bộ trong cùng một record.
|
|
|
|
Hiện chưa nổ vì **chưa có client nào gọi PUT** (xem F3). Nhưng nó là mìn đặt sẵn cho lúc nối dây FE: nếu người nối dây liệt kê tay payload và quên 1 dòng (đúng cái bẫy mà comment `fe-admin/.../ApprovalWorkflowsV2Page.tsx:617-620` vừa tự cảnh báo cho POST), thì kết quả không phải "cờ mới không lưu" mà là "**cờ CŨ bị xoá**".
|
|
|
|
**Đề xuất sửa.** Đổi 7 cờ sang `bool?` với ngữ nghĩa `null = giữ nguyên`, hoặc thêm test hợp đồng chốt rằng payload thiếu field ⇒ 400 chứ không phải ghi default.
|
|
|
|
**Tiêu chí nghiệm thu.** Test: level đang có `AllowApproverEditBudget=true`, gửi payload JSON **không có** key đó → assert giá trị sau lệnh **vẫn `true`** (hoặc lệnh 400).
|
|
|
|
---
|
|
|
|
## 🟠 F5 — MED — `DetachTrackedOpinionsOn` quét theo TÊN CỘT: bắt thừa sang entity không phải chữ ký, và có thể nuốt một `Added` thật
|
|
|
|
**File:** `ApprovalWorkflowV2AdminFeatures.cs:936-949`, gọi tại `:764-765`
|
|
|
|
```csharp
|
|
const string fkName = nameof(PurchaseEvaluationLevelOpinion.ApprovalWorkflowLevelId);
|
|
var stale = ((DbContext)db).ChangeTracker.Entries()
|
|
.Where(e => e.State != EntityState.Detached
|
|
&& e.Metadata.FindProperty(fkName) is not null
|
|
&& e.Property(fkName).CurrentValue is Guid levelId
|
|
&& levelIds.Contains(levelId))
|
|
.ToList();
|
|
foreach (var entry in stale) entry.State = EntityState.Detached;
|
|
```
|
|
|
|
**Bắt thừa — đo được:** `grep -rln "ApprovalWorkflowLevelId" src/Backend/**/*.cs` trả **9 file nguồn**, trong đó ngoài 7 entity `*LevelOpinion` còn có `PurchaseEvaluations/PurchaseEvaluationFeatures.cs`, `Contracts/ContractFeatures.cs`, `Office/*Features.cs` (chỉ là truy vấn, không phải entity) — **nhưng** phép quét không dùng danh sách entity, nó dùng `e.Metadata.FindProperty(name)`, tức bắt **mọi** entity type (kể cả shadow property) có cột trùng tên. Hôm nay tập đó = đúng 7 bảng chữ ký, nên hành vi đúng. Ngày mai thêm một entity bất kỳ có FK cùng tên (ví dụ bảng "phân công thay thế người duyệt", "nhật ký cấp duyệt") thì nó **bị detach im lặng** — mất luôn thay đổi đang chờ lưu của entity đó.
|
|
|
|
**Nuốt `Added`:** hàm không phân biệt state. Nếu tracker đang giữ một opinion state `Added` trỏ vào Level sắp gỡ, `entry.State = Detached` **huỷ luôn lệnh INSERT** mà không báo gì. Trong luồng hiện tại handler không add opinion nên chưa nổ, nhưng `IApplicationDbContext` là **scoped theo request**, và MediatR pipeline behavior chạy chung context — điều kiện đủ để nó nổ không nằm trong file này.
|
|
|
|
**Ẩn lỗi thật:** comment `:930-934` biện hộ "guard F-5 đã xác nhận 0 chữ ký còn hiệu lực ⇒ mọi bản sao tracked đều là rác". Lập luận này đúng **chỉ khi** guard và tracker nhìn cùng một sự thật. Guard đọc `AsNoTracking` từ DB; một opinion vừa được tạo trong cùng request mà **chưa** SaveChanges thì DB chưa thấy ⇒ guard đếm 0 ⇒ detach xoá sổ nó. Đây đúng là "chỗ ta KHÔNG muốn EF quên".
|
|
|
|
**Đề xuất sửa.** (i) Liệt kê **tường minh** 7 CLR type thay vì so tên cột (`e.Entity is PurchaseEvaluationLevelOpinion or ContractLevelOpinion or …`) — mất tính "tự phủ" nhưng được tính đóng, và nếu thêm bảng thứ 8 thì compiler/test bắt được thay vì im lặng; (ii) chỉ detach entry có `State == Unchanged` hoặc `Deleted`, còn gặp `Added`/`Modified` thì **ném** để lộ ra bất thường thay vì nuốt.
|
|
|
|
**Tiêu chí nghiệm thu.** Test: tracker giữ 1 opinion `Added` trỏ Level sắp gỡ → gọi Update → assert **ném** (hoặc opinion vẫn được insert), không phải biến mất.
|
|
|
|
---
|
|
|
|
## 🟢 F6 — LOW — `u.FullName ?? u.Email` có thể ra `null` ⇒ thông điệp 403 lòi dấu phẩy trống
|
|
|
|
**File:** `src/Backend/SolutionErp.Infrastructure/Services/PurchaseEvaluationWorkflowService.cs:709-714`
|
|
|
|
```csharp
|
|
var resolved = await userManager.Users.AsNoTracking()
|
|
.Where(u => allowedIdList.Contains(u.Id))
|
|
.Select(u => u.FullName ?? u.Email) // ← List<string?>
|
|
.ToListAsync(ct);
|
|
var names = resolved.Count > 0 ? string.Join(", ", resolved) : string.Join(", ", allowedIdList);
|
|
```
|
|
|
|
`User : IdentityUser<Guid>` ⇒ `Email` là `string?`. Nếu một NV có `FullName = null` **và** `Email = null` thì phần tử là `null`, `string.Join` render thành chuỗi rỗng ⇒ thông điệp thành `"… (Nguyễn A, , Trần B)"`. Fallback `resolved.Count > 0` **không cứu** được ca này vì Count vẫn > 0.
|
|
|
|
Tệ hơn một chút: nếu Cấp có 3 người mà **1 người đã bị xoá khỏi bảng Users** thì `resolved.Count = 2` > 0 ⇒ nhánh fallback không chạy ⇒ thông điệp liệt kê **thiếu người**, người đọc tưởng chỉ có 2 người duyệt được. Không phải lỗi bảo mật, nhưng là sai-im-lặng nhẹ đúng họ lỗi mà F-6 đang đi chữa (GUID khó hiểu → tên).
|
|
|
|
**Đề xuất.** `.Select(u => u.FullName ?? u.Email ?? u.Id.ToString())` (khuôn này đã dùng ĐÚNG, có đủ 3 nấc, ở `ResolveUserNamesAsync` — `ApprovalWorkflowV2AdminFeatures.cs:957` — tức chính diff này chứa cả bản đúng lẫn bản thiếu, cách nhau 2 file), và đối chiếu số lượng: `resolved.Count == allowedIdList.Count` thì mới bỏ GUID, thiếu thì nối thêm GUID của người không tra được.
|
|
|
|
---
|
|
|
|
## 🟢 F7 — LOW/INFO — "lộ TÊN người duyệt cho người vừa bị từ chối": KHÔNG phải rò rỉ mới, vì tên+email vốn đã công khai cho mọi tài khoản đăng nhập
|
|
|
|
**Đây là câu lead hỏi là quan trọng nhất. Tao trả lời bằng đo, không bằng cảm giác.**
|
|
|
|
Kênh rò rỉ **đã tồn tại từ trước**, rộng hơn hẳn:
|
|
|
|
- `src/Backend/SolutionErp.Api/Controllers/ApprovalWorkflowsV2Controller.cs:21-26` — `GET /api/approval-workflows-v2` chỉ có `[Authorize]` **trần** (class-level, cố ý theo gotcha #44), **không** policy.
|
|
- Query trả về `AwLevelDto` gồm `ApproverFullName` **và `ApproverEmail`** — `ApprovalWorkflowV2AdminFeatures.cs:188-193`.
|
|
|
|
⇒ **Bất kỳ tài khoản đăng nhập nào** cũng GET được toàn bộ cây quy trình kèm **họ tên + email** của mọi người duyệt, ở mọi module. So với nền đó, việc thông điệp 403 in ra tên của đúng 1 Cấp là **tập con thực sự** của thứ đã công khai — delta rò rỉ ≈ 0.
|
|
|
|
Thêm nữa, FE **đang** hiển thị tên người duyệt cho chính người không đến lượt: banner amber "Không phải lượt bạn — chỉ {NV X / Y} duyệt được" (khuôn V2, xem skill `contract-workflow` §FE UX V2-aware). Nghĩa là tên đã ở trên màn hình người dùng đó rồi.
|
|
|
|
**Kết luận trục này: KHÔNG chặn deploy.** Nhưng ghi lại một mệnh đề đúng: *nếu* sau này siết `GET /api/approval-workflows-v2` bằng policy (việc nên làm), thì **phải nhớ siết cả chỗ này**, nếu không thông điệp 403 trở thành cửa hậu đọc tên. Ghi vào chỗ nào đọc được — hiện comment `:704-708` chỉ giải thích "GUID khó hiểu", không nhắc ràng buộc này.
|
|
|
|
**Chi phí query trong nhánh ném:** chấp nhận được. Nhánh này chỉ chạy khi người dùng bấm Duyệt sai lượt (hiếm, do FE đã disable nút), 1 round-trip `WHERE Id IN (≤3)`, và đằng nào cũng sắp ném 403 — không nằm trên đường nóng.
|
|
|
|
---
|
|
|
|
## 🔴 F8 — HIGH (cổng deploy, không phải cổng mã) — 2/3 file migration đang UNTRACKED; commit kiểu `-a`/`add -u` sẽ bỏ chúng lại và **hạ toàn bộ 7 module**
|
|
|
|
**Bằng chứng `git status --porcelain`:**
|
|
|
|
```
|
|
?? src/Backend/SolutionErp.Infrastructure/Persistence/Migrations/20260727033522_AddPeAllowApproverDelete.Designer.cs
|
|
?? src/Backend/SolutionErp.Infrastructure/Persistence/Migrations/20260727033522_AddPeAllowApproverDelete.cs
|
|
?? tests/SolutionErp.Infrastructure.Tests/Application/UpdateAwDefinitionTests.cs
|
|
M src/Backend/SolutionErp.Infrastructure/Persistence/Migrations/ApplicationDbContextModelSnapshot.cs
|
|
```
|
|
|
|
**Điều sai.** Trong bộ 3 file của gotcha #17, **file thứ ba (snapshot) đã tracked** còn **hai file kia thì chưa**. `git commit -a` và `git add -u` chỉ nạp file đã tracked. Kết quả nếu lỡ tay:
|
|
|
|
- Snapshot + entity (`ApprovalWorkflow.cs`) + config (`ApprovalWorkflowConfiguration.cs`) **vào commit** ⇒ model của EF **có** cột `AllowApproverDelete`;
|
|
- File migration **không vào commit** ⇒ `DbInitializer.cs:64` `await db.Database.MigrateAsync()` chạy trên prod và **không thấy gì để áp** ⇒ cột không bao giờ được tạo;
|
|
- Mọi truy vấn chạm `ApprovalWorkflowLevels` ném `Invalid column name 'AllowApproverDelete'`.
|
|
|
|
Blast radius = **toàn bộ** người dùng schema V2: PE, Hợp đồng, và 5 module Văn phòng số (Đề xuất / Nghỉ phép / Tăng ca / Công tác / Xe). Không phải một tính năng hỏng — là **duyệt phiếu ngừng hoạt động toàn hệ**.
|
|
|
|
Đây là sai lầm **rẻ nhất để phạm và đắt nhất để lãnh** trong toàn bộ đợt này, vì nó không để lại dấu vết nào ở local (local đã áp migration rồi, xem F9) và build/test đều xanh.
|
|
|
|
**Đề xuất.** Trước khi commit: `git add` **đích danh** 3 đường dẫn trên, rồi `git status --porcelain | grep '^??'` phải **rỗng** cho vùng `Migrations/` và `tests/`. Sau khi commit: `git show --stat HEAD | grep AddPeAllowApproverDelete` phải ra **2 dòng**.
|
|
|
|
---
|
|
|
|
## 🟠 F9 — CLEAN (migration) + 1 LOW về tên — *thách-CLEAN: tao thử phá bằng 4 phép, trượt cả 4*
|
|
|
|
**Kết luận: bản migration này AN TOÀN để chạy trên prod có dữ liệu thật.** Không phải "đọc thấy ổn" — dưới đây là 4 cách tao đã thử phá:
|
|
|
|
**Phép 1 — 3-file rule (gotcha #17).** Đủ cả ba: `20260727033522_AddPeAllowApproverDelete.cs`, `…Designer.cs`, `ApplicationDbContextModelSnapshot.cs`. (Vấn đề *tracked hay chưa* tách riêng ở F8 — đó là lỗi git, không phải lỗi migration.)
|
|
|
|
**Phép 2 — Designer ⟂ Snapshot có lệch không?** So từng dòng, hai khối **trùng khít**:
|
|
|
|
```csharp
|
|
b.Property<bool>("AllowApproverDelete")
|
|
.ValueGeneratedOnAdd()
|
|
.HasColumnType("bit")
|
|
.HasDefaultValue(false);
|
|
```
|
|
|
|
(`…Designer.cs:198-201` ⟂ `ApplicationDbContextModelSnapshot.cs`, cùng nội dung.) Lệch snapshot là nguồn "migration kế tiếp sinh ra lệnh lạ" — không có ở đây.
|
|
|
|
**Phép 3 — đã chạy thật trên SQL Server chưa, hay mới chỉ là ý định?** Đo bằng `sqlcmd` trên LocalDB Dev, không tin file:
|
|
|
|
```
|
|
__EFMigrationsHistory (3 bản mới nhất):
|
|
20260727033522_AddPeAllowApproverDelete ← ĐÃ ÁP
|
|
20260717032812_AddPeApprovedBudgetSnapshot
|
|
20260714035636_AddPeApprovedPriceRange
|
|
|
|
INFORMATION_SCHEMA.COLUMNS / ApprovalWorkflowLevels:
|
|
AllowApproverDelete bit NO (CONVERT([bit],(0)))
|
|
```
|
|
|
|
Cột ra đúng hình dạng **y hệt 8 cờ anh em** đang sống (`AllowReturnOneLevel`… `AllowApproverFinalize` đều `bit NOT NULL DEFAULT CONVERT([bit],(0))`, riêng `AllowReturnToDrafter` default `(1)`). Tức nó không phải cấu trúc mới lạ, mà là dòng thứ 9 của một khuôn đã chạy 4 lần trước đó (Mig 29/30/31/58).
|
|
|
|
**Phép 4 — có làm khoá bảng / rewrite trên prod không?** `AddColumn<bool>(nullable: false, defaultValue: false)` là **hằng số**, nên SQL Server 2012+ xử lý như thao tác **chỉ-metadata**, không viết lại bảng. Cộng thêm bảng cực nhỏ (`SELECT COUNT(*) FROM ApprovalWorkflowLevels` = **6** trên Dev; prod cùng bậc độ lớn — quy trình duyệt chỉ vài chục dòng Cấp). Không có nguy cơ timeout/khoá kéo dài.
|
|
|
|
**`Down()` reversible:** có — `DropColumn`. Lưu ý đúng mức: rollback làm **mất giá trị cờ đã tick**, nhưng vì default là `false` nên rollback đưa hệ về đúng trạng thái trước tính năng. Chấp nhận được.
|
|
|
|
**Migration chạy tự động lúc khởi động** (`DbInitializer.cs:64`), nên deploy không cần bước tay. Mặt trái: migration hỏng sẽ chặn app khởi động — bản này không thể hỏng vì lý do ở Phép 4.
|
|
|
|
> 🟢 **F9-LOW (tên gọi).** `AddPeAllowApproverDelete` — tiền tố `Pe` gây hiểu nhầm: cột nằm trên `ApprovalWorkflowLevels`, bảng **dùng chung ≥7 module**, không riêng PE. Người sau grep "ai sở hữu cột này" sẽ bị dẫn sai hướng. Không đáng sửa trước deploy (đổi tên migration đã áp còn tệ hơn), chỉ nên ghi 1 dòng vào changelog.
|
|
|
|
---
|
|
|
|
## 🟢 F10 — CLEAN (Validator T1/T2 + bất biến Version/IsActive/Id) — *thách-CLEAN: 11 đường lách đã thử, tắc cả 11; cộng 1 phép quét gián tiếp*
|
|
|
|
### (a) Handler có đúng KHÔNG tăng `Version` / KHÔNG đụng `IsActive` / KHÔNG đổi `Id` không?
|
|
|
|
Tao **không** trả lời bằng cách đọc lướt. Tao liệt kê **toàn bộ** phép ghi vào aggregate root trong thân handler:
|
|
|
|
```
|
|
$ sed -n '501,830p' ApprovalWorkflowV2AdminFeatures.cs | grep -n "def\.[A-Za-z]* *="
|
|
197: def.Name = request.Name;
|
|
198: def.Description = request.Description;
|
|
280: def.UpdatedAt = dateTime.UtcNow;
|
|
```
|
|
|
|
**Đúng 3 phép ghi, hết.** Không có `def.Version`, `def.IsActive`, `def.Id`, `def.Code`, `def.ApplicableType`, `def.CeoApprovalThreshold`, `def.IsUserSelectable`. Đây là phép **liệt kê vét cạn**, không phải kiểm điểm.
|
|
|
|
Rồi tao quét thêm **đường ghi gián tiếp** (đúng lớp gotcha #81-EXT — sửa qua biến trung gian nên grep trực tiếp không thấy):
|
|
|
|
```
|
|
$ sed -n '501,830p' … | grep -n "= def;|SetValues|Entry(|CurrentValues|\.Version|\.IsActive|\.IsUserSelectable|\.Code *=|ApplicableType *="
|
|
316: Summary = $"Sửa quy trình duyệt tại chỗ: {def.Code} v{def.Version} (giữ nguyên phiên bản)"
|
|
```
|
|
|
|
Hit duy nhất là một phép **ĐỌC** trong chuỗi nội suy. Không có alias, không có `SetValues`, không có `Entry().CurrentValues`. ⇒ Ba bất biến **được chứng minh**, không phải được tin.
|
|
|
|
### (b) 11 đường lách đã thử — tắc cả 11
|
|
|
|
| # | Cách phá | Chốt chặn | Vị trí (đã mở kiểm) |
|
|
|---|---|---|---|
|
|
| 1 | Hoán vị Order giữa 2 dòng (giữ nguyên TẬP) | Order từng dòng bất biến | `:575` |
|
|
| 2 | Thêm Cấp mới (Order chưa có) | `SetEquals` cuối vòng | `:621` |
|
|
| 3 | Rút hết người khỏi một Cấp | nhánh `lost` trong cùng khối | `:621-628` |
|
|
| 4 | Chuyển người sang Bước khác | `unknown` (dict theo từng Bước) | `:565` |
|
|
| 5 | Gửi trùng Id dòng Cấp | `Distinct().Count()` | `:560` |
|
|
| 6 | Bỏ sót / thêm Bước | `SetEquals` Id Bước | `:539` |
|
|
| 7 | Đảo thứ tự Bước (giữ TẬP) | so `reqStep.Order != dbStep.Order` | `:554` |
|
|
| 8 | Đổi ngưỡng CEO | echo nghiêm (null ≠ bỏ qua) | `:529` |
|
|
| 9 | Bật/tắt F5 (và F2 ngay dưới) | echo, lệch ⇒ 409 | `:580` (+`:584`) |
|
|
| 10 | Người MỚI mang sẵn cờ F2/F5 | chặn riêng nhánh `else` | `:594` |
|
|
| 11 | Bỏ người **đang giữ** cờ F2/F5 | quét `removedHere` | `:609` |
|
|
|
|
Cộng thêm: NV được trỏ tới phải **tồn tại thật** (`:643`, chặn FK 547 hoá 500), và người có chữ ký sống thì **không bỏ được / không đổi người được** (`:659`). Tao không tìm được đường thứ 12.
|
|
|
|
### 🟠 F10a — MED — đổi **Mô tả** quy trình: ghi thẳng, KHÔNG sinh vết
|
|
|
|
`:698` `def.Description = request.Description;` — gán vô điều kiện, và **không** có `changes.Add` tương ứng (đối chiếu `:696` cho `Name` thì CÓ). Hệ quả kép:
|
|
|
|
- Đổi mô tả ⇒ `changes` rỗng ⇒ khối changelog `:790-821` **không chạy** ⇒ phiếu đang chạy **không có vết nào**;
|
|
- Mô tả là thứ người duyệt **đọc trên phiếu**, nên đây không phải trường trang trí.
|
|
|
|
**Đề xuất:** thêm `if (def.Description != request.Description) changes.Add(...)` — một dòng, đối xứng với `Name`.
|
|
|
|
### 🟠 F10b — MED — thêm người vào **Cấp ĐÃ QUA** không bị chặn, và màn hình sẽ hiển thị người đó là "đã duyệt"
|
|
|
|
Handler cho phép thêm dòng vào bất kỳ Cấp nào miễn Order đã tồn tại — kể cả Cấp mà con trỏ phiếu **đã đi qua**. Phía hiển thị, `ComputeLevelStatus` là **số học con trỏ thuần**, không hề tra chữ ký:
|
|
|
|
```csharp
|
|
// PurchaseEvaluationFeatures.cs:1135-1147
|
|
if (stepIdx0 < currentIdx.Value) return "Done";
|
|
if (stepIdx0 == currentIdx.Value) {
|
|
if (levelOrder < currentLevel.Value) return "Done";
|
|
…
|
|
}
|
|
```
|
|
|
|
⇒ Admin thêm ông A vào Bước 1 Cấp 1 trong khi phiếu đã ở Bước 2 ⇒ luồng duyệt hiển thị Cấp đó **"Done"** ⇒ người đọc hiểu là **ông A đã ký**, trong khi ông A chưa từng chạm phiếu. Trên chứng từ tài chính, đó là **chữ ký ma trên màn hình**.
|
|
|
|
Đã nêu ở lượt soi spec (mục M1) và diff này **chưa xử lý**. Không chặn deploy đợt 1 (cần thao tác admin bất thường mới lộ), nhưng phải vào sổ trước khi mở tính năng cho nhiều admin.
|
|
|
|
**Đề xuất:** hoặc chặn thêm người vào Cấp đã qua trong `toAdd`, hoặc để `ComputeLevelStatus` đối chiếu `LevelOpinions` thay vì thuần con trỏ.
|
|
|
|
### 🟢 F10c — LOW — cho sửa `Step.DepartmentId` là AN TOÀN, đây là cách tao kiểm
|
|
|
|
Comment ở `:412` khẳng định "BE chỉ dùng làm hint, không khớp người duyệt". Tao không tin lời khẳng định, tao grep toàn `src/Backend`:
|
|
|
|
- `PurchaseEvaluationWorkflowService.cs:1117-1134` và `ContractWorkflowService.cs:131-148` — **đều nằm trong nhánh V1 legacy** (`matchByDeptLevel`, đối tượng là `WorkflowStep` của Mig 21), **không phải** đường V2. Đường V2 khớp người **chỉ** bằng `ApproverUserId` (`PurchaseEvaluationWorkflowService.cs:702`).
|
|
- `PurchaseEvaluationFeatures.cs:1187-1188` / `:1240-1241` — thuần **DTO hiển thị** (map sang tên phòng).
|
|
|
|
⇒ Khẳng định đúng. Ghi chú nhỏ: `changes.Add` cho trường này chỉ ghi *"đổi phòng ban phụ trách"* (`:704`) mà **không ghi từ phòng nào sang phòng nào** — vết yếu hơn các trường khác.
|
|
|
|
---
|
|
|
|
## 🟠 F11 — MED — một ca test **XANH GIẢ**: `F16_…SafeUpdate_Succeeds_AndWritesNoPeChangelog`
|
|
|
|
**File:** `tests/SolutionErp.Infrastructure.Tests/Application/UpdateAwDefinitionTests.cs:753-754`
|
|
|
|
```csharp
|
|
(await h.Db.PurchaseEvaluationChangelogs.AsNoTracking().CountAsync())
|
|
.Should().Be(0, "quy trình module Đề xuất KHÔNG được đẻ vết vào changelog của PE");
|
|
```
|
|
|
|
**Điều sai — ca này không mang thông tin nào.** Cách tao chứng minh (không phải đọc thấy nghi ngờ): truy **mọi** phép ghi vào `PurchaseEvaluationChangelogs` trong handler → chỉ có **một**, ở `:807`, nằm trong `foreach (var pe in affected)`, mà `affected` đến từ `db.PurchaseEvaluations.Where(p => p.ApprovalWorkflowId == def.Id …)` (`:794-800`). Test này seed **0 phiếu PE** (chỉ có một `Proposal`, `:730-740`). Vậy:
|
|
|
|
- **Thế giới A** (còn cổng `def.ApplicableType is DuyetNcc or DuyetNccPhuongAn`): `affected` rỗng ⇒ count = 0.
|
|
- **Thế giới B** (xoá cổng đi): `affected` **vẫn** rỗng ⇒ count = 0.
|
|
|
|
Hai thế giới, cùng một quan sát ⇒ phép đo **không phân biệt được** chúng ⇒ xoá cổng module đi test vẫn xanh. Nó đang đo *"không có phiếu PE nào tồn tại"*, không đo *"cổng module hoạt động"*.
|
|
|
|
**Đề xuất.** Cho ca này răng bằng cách seed **một phiếu PE ghim đúng workflow ProposalGeneral đó** (`SeedPeAsync(h.Db, wf.Id, …)` — hàm đã có sẵn ở `:108`), rồi mới assert `Count == 0`. Khi đó gỡ cổng ⇒ test đỏ.
|
|
|
|
**Tiêu chí nghiệm thu:** tạm gỡ điều kiện `def.ApplicableType is …` khỏi `:790-792`, chạy lại ca này — **phải ĐỎ**. Hiện tại nó xanh.
|
|
|
|
---
|
|
|
|
## 🟢 F12 — CLEAN (chất lượng các ca test còn lại) — *thách-CLEAN: 3 phép thử, gồm cả phép trả lời thẳng câu "double-insert"*
|
|
|
|
**Phép 1 — assert có phải chỉ là tiếng vọng của bộ nhớ không?** Cả file dùng **chung một** `h.Db`. Nếu `ResolveLikeRunningPeAsync` mà tracking thì nó sẽ trả về **chính những đối tượng handler vừa sửa trong RAM** ⇒ mọi assert "phiếu thấy thay đổi" thành vô nghĩa. Nó dùng `AsNoTracking()` (`:143`) ⇒ EF bỏ qua identity map, đọc lại từ DB thật. **Đúng.** Đây là chỗ dễ hỏng nhất và nó không hỏng.
|
|
|
|
**Phép 2 — trả lời trục "`db.ApprovalWorkflowLevels.Add` + `step.Levels.Add` có sinh double-insert?"** Không. Cùng **một tham chiếu đối tượng** được đưa vào DbSet và vào collection điều hướng của cha đang tracked ⇒ EF thấy **một** entity ở trạng thái `Added`, không phải hai. Và điều quan trọng hơn: **đã có test bắt được nếu sai** — `F12` (`:262-263`) assert đúng **2** dòng ở Cấp 1:
|
|
|
|
```csharp
|
|
live.Steps[0].Levels.Where(l => l.Order == 1).Select(l => l.ApproverUserId)
|
|
.Should().BeEquivalentTo(new[] { alice.Id, bob.Id });
|
|
```
|
|
|
|
Double-insert sẽ ra **3** dòng ⇒ đỏ. Đây là bằng chứng, không phải suy luận.
|
|
|
|
**Phép 3 — các ca 409 có mù trước lỗi "ném SAU khi đã ghi" không?** Không. Mọi ca `F13*` assert **hai vế**: có ném **và** DB y nguyên. Mạnh nhất là `F13b_OmitCeoApprovalThreshold` (`:443`) — nó gửi kèm `Name = "Tên mới"` rồi assert tên **vẫn là tên cũ**, tức chứng minh **không phần nào** của lệnh lọt xuống trước khi ném.
|
|
|
|
Ngoài ra `F12` đo bằng **hành vi thật** (trước: `ForbiddenException`; sau: cùng người, cùng lời gọi, con trỏ tiến sang Cấp 2) chứ không bằng "DB có row mới" — đúng thứ spec cần. `F14` có khối chứng-nhân FK thật (đã tự dọn tracker để đo đúng tầng DB). `F16_Proposal…Blocked409` chạm module Đề xuất thật.
|
|
|
|
> 🔴 **F12-CẢNH BÁO CHO WAVE 5 (đang vá F2) — đọc trước khi sửa:**
|
|
> `UpdateAwDefinitionTests.cs:607-609` đang **khoá hành vi hiện tại**:
|
|
> ```csharp
|
|
> (await h.Db.PurchaseEvaluationLevelOpinions.IgnoreQueryFilters()
|
|
> .CountAsync(o => o.ApprovalWorkflowLevelId == lvlBob.Id)).Should().Be(0);
|
|
> ```
|
|
> Ca này **chốt rằng purge PHẢI xoá sạch chữ ký của phiếu xoá mềm** — tức đúng **chiều ngược lại** với đề xuất vá F2. Khi vá F2, ca này **sẽ ĐỎ**. Đó **không phải hồi quy**: phải viết lại chủ đích của ca (đổi thành "chữ ký của phiếu xoá mềm được GIỮ, và lệnh bị 409"), **không được** nới assert cho nó xanh. Nếu ai đó làm nó xanh bằng cách bỏ dòng assert thì F2 coi như chưa vá.
|
|
|
|
---
|
|
|
|
## 🟢 F13 — CLEAN (hồi quy) — *thách-CLEAN: 3 phép, và phép thứ 3 lòi ra 1 chỗ vá NỬA*
|
|
|
|
**Phép 1 — PUT mới có đụng client cũ không? KHÔNG.** Bảng route của controller sau khi thêm: `GET` (không template) · `POST` (không template) · `PUT {id:guid}` ← mới · `PATCH {id:guid}/user-selectable` · `DELETE {id:guid}`. Cặp (verb, template) của PUT **chưa từng tồn tại**, và không có `[HttpPut]` nào khác trong file. Phía client: `grep -rn "approval-workflows-v2" fe-admin/src fe-user/src` ra **29 hit**, phần ghi đúng **3** (`api.delete:286`, `api.patch:297`, `api.post:589`) — **không client nào** gọi PUT. Không thể hồi quy thứ chưa ai gọi.
|
|
|
|
**Phép 2 — đổi message 403 có làm đỏ test nào không? KHÔNG.** Tao grep chuỗi `"danh sách NV duyệt"` toàn repo trên `*.cs|*.ts|*.tsx` → đúng **3 hit**: 2 service + đúng **1** test là `ContractWorkflowServiceApproveV2Tests.cs:457`:
|
|
|
|
```csharp
|
|
.WithMessage("*Bước 1*Cấp 1: bạn không có trong danh sách NV duyệt*");
|
|
```
|
|
|
|
Nó dùng **ký tự đại diện** và dừng **trước** phần `({names})` — tức không hề chạm khúc bị sửa. Quan trọng hơn: nó đo `ContractWorkflowService`, file **không nằm trong diff**. Hai lý do độc lập ⇒ không thể đỏ.
|
|
|
|
**Phép 3 — module nào có nguy cơ vỡ?** Tao kiểm bán kính bằng cách đếm ngược từ **cấu hình FK** chứ không từ danh sách trong mã:
|
|
|
|
```
|
|
$ grep -A1 "HasForeignKey(x => x.ApprovalWorkflowLevelId)" …/Configurations/
|
|
→ 7 cấu hình, TẤT CẢ OnDelete(DeleteBehavior.Restrict):
|
|
PurchaseEvaluationLevelOpinion · ContractLevelOpinion · ProposalLevelOpinion
|
|
LeaveRequestLevelOpinion · OtRequestLevelOpinion · TravelRequestLevelOpinion
|
|
VehicleBookingLevelOpinion
|
|
```
|
|
|
|
**Đúng 7 — khớp tuyệt đối** với 7 bảng mà `CountLiveOpinionsAsync:830-884` và `PurgeOrphanOpinionsAsync:886-918` liệt kê. Nếu có bảng thứ 8 thì phép grep này phải ra 8; nó ra 7. Đồng thời `Level→Step` là **Cascade** và `Level→User` là **Restrict** (`ApprovalWorkflowConfiguration.cs:65-75`), nên `Remove(level)` chỉ vướng đúng 7 bảng đó. ⇒ Không module nào vỡ vì thiếu quét.
|
|
|
|
### 🟠 F13a — MED — vá NỬA: câu 403 sinh đôi ở `ContractWorkflowService` **chưa được vá**
|
|
|
|
**File:** `src/Backend/SolutionErp.Infrastructure/Services/ContractWorkflowService.cs:264`
|
|
|
|
```csharp
|
|
var names = string.Join(", ", allowedUserIds); // ← vẫn in GUID trần
|
|
throw new ForbiddenException(
|
|
$"Bước {currentIdx + 1} ({currentStep.Name}) — Cấp {currentLevelOrder}: bạn không có trong danh sách NV duyệt ({names}).");
|
|
```
|
|
|
|
Đây là **cùng một câu, cùng một lỗi, cùng một schema V2**, chỉ khác module. F-6 vá `PurchaseEvaluationWorkflowService.cs:709-716` mà bỏ lại đây ⇒ người duyệt Hợp đồng vẫn nhận một dãy GUID không đọc được. Đúng lớp lỗi "được chỉ đích danh 1 chỗ thì phải grep cả lớp".
|
|
|
|
Không chặn deploy (đợt 1 là PE), nhưng nên gộp vào cùng commit vì chi phí bằng 0 và để lâu sẽ thành nợ vô hình.
|
|
|
|
**Đề xuất.** Bê nguyên khối `:709-716` sang, **kèm** sửa F6 (`?? u.Id.ToString()` ba nấc) cho cả hai chỗ một lượt.
|
|
|
|
---
|
|
|
|
---
|
|
|
|
## ⚠️ GHI CHÚ TRÔI DÒNG — file bị sửa TRONG LÚC soi (wave 5 vá song song)
|
|
|
|
Lúc kiểm lại anchor cuối cùng, `ApprovalWorkflowV2AdminFeatures.cs` có mtime **12:18:13** trong khi tao kiểm lúc **12:19:24** — tức file vừa đổi 71 giây trước. Đây là wave 5 đang vá F1/F2/F3 đúng như lead báo. Tao **không** soi lại bản vá đó (ngoài phạm vi lượt này, và lead đã nói sẽ tự verify bằng build + test + grep), nhưng ghi lại hai sự thật đo được để lượt sau không bị lạc:
|
|
|
|
**(1) F1 có dấu hiệu ĐÃ được vá đúng hướng.** Đo bằng grep, không phải suy đoán:
|
|
|
|
```
|
|
695: await using var tx = await context.Database.BeginTransactionAsync(ct);
|
|
704: purgedOrphans = await PurgeOrphanOpinionsAsync(…);
|
|
843: await db.SaveChangesAsync(ct);
|
|
847: await tx.CommitAsync(ct);
|
|
```
|
|
|
|
Hình dạng đúng: transaction mở **trước** purge, commit **sau** `SaveChanges`, và `await using` bảo đảm rollback khi ném. Đây là phương án (a) tao đề xuất ở F1. **Chưa** kiểm sâu (chưa soi `context` được lấy từ đâu, chưa soi tương tác với `IApplicationDbContext`), nên đừng đọc dòng này là "F1 đã PASS review".
|
|
|
|
**(2) Bản đồ trôi dòng.** Mọi anchor **≤ `:659`** vẫn ĐÚNG NGUYÊN (đã kiểm lại: `:529`, `:539`, `:554`, `:575` không đổi) — nghĩa là **toàn bộ bảng 11 đường lách ở F10 vẫn mở được**. Các anchor **sau `:660`** trôi **+21 dòng**:
|
|
|
|
| Mốc | Số cũ trong báo cáo này | Số hiện tại |
|
|
|---|---|---|
|
|
| gọi `PurgeOrphanOpinionsAsync` | `:683` | `:704` |
|
|
| `def.Description = …` | `:698` | `:719` |
|
|
| `changes.Add` đổi phòng ban | `:704` | `:725` |
|
|
| `SaveChangesAsync` của handler | `:822` | `:843` |
|
|
| thân `PurgeOrphanOpinionsAsync` | `:886` | `:911` |
|
|
| `DetachTrackedOpinionsOn` | `:936` | `:961` |
|
|
| khối changelog (gate module) | `:790` | `:811` |
|
|
|
|
Các FLAG **F4, F5, F10a, F10c, F11** trỏ vào vùng sau `:660` ⇒ cộng **+21** khi mở file ở bản hiện tại. Bản chất phát hiện **không đổi** (tao đã đối chiếu bằng nội dung dòng, không chỉ bằng số).
|
|
|
|
---
|
|
|
|
# TỔNG KẾT
|
|
|
|
## Điểm làm ĐÚNG, đáng giữ nguyên
|
|
|
|
1. **`CountLiveOpinionsAsync` viết `!p.IsDeleted` TƯỜNG MINH thay vì dựa global query filter.** Tao kiểm `grep HasQueryFilter` toàn thư mục Configurations: chỉ PE và Contract có filter; **5 bảng Office KHÔNG có**. Nếu tác giả dựa vào filter thì PE đúng còn 5 module kia sai âm thầm. Đây là chi tiết dễ sai nhất trong cả file và nó **đúng**.
|
|
2. **Bảy bảng opinion được quét đủ**, khớp tuyệt đối với 7 cấu hình FK Restrict (F13 Phép 3).
|
|
3. **Hai cờ đổi-đường-đi xử lý kiểu ECHO** (`bool?`, lệch ⇒ 409, **không bao giờ ghi**) — cộng ca test `F13b_OmittedFinalizeEcho` chốt rằng bỏ trống thì **giữ nguyên**, không bị `?? false` xoá ngầm.
|
|
4. **`CeoApprovalThreshold` cố tình KHÔNG dùng "null = bỏ qua"**, kèm lý do viết ngay tại chỗ (`:416-422`): null **là** một giá trị có nghĩa. Đây là chỗ 9/10 lần người ta làm sai.
|
|
5. **Phép thử T1 siết mạnh hơn set-equality** (Order từng dòng bất biến), và có test `F13_SwapStepOrders` chứng minh set-equality một mình sẽ lọt.
|
|
6. **`F12` đo bằng hành vi thật** với nửa contrast (trước Forbidden / sau duyệt được) — chống xanh-rỗng đúng cách.
|
|
7. **Migration là dòng thứ 9 của một khuôn đã chạy 4 lần**, không phải cấu trúc mới (F9 Phép 3).
|
|
8. **Comment tự khai giới hạn** (`:786-789`: 5 module Office chưa có changelog) thay vì giấu. Khai giới hạn đúng chỗ đáng giá hơn một đoạn mã đẹp.
|
|
|
|
## Bảng FLAG
|
|
|
|
| # | Mức | Nội dung | Vị trí |
|
|
|---|---|---|---|
|
|
| F1 | HIGH | `ExecuteDelete` ngoài transaction, trước `SaveChanges` ⇒ mất chữ ký không hoàn tác | `AdminFeatures.cs:683` ⟂ `:822` |
|
|
| F2 | HIGH | Purge xoá CỨNG chữ ký của phiếu mới **xoá mềm** ⇒ phá "khôi phục phiếu" (đợt 2) | `AdminFeatures.cs:889-915` |
|
|
| F3 | HIGH | Endpoint PUT **không có dây FE** ⇒ mục tiêu §F.0 không dùng được trong UAT | `Controller.cs:45` ⟂ `…V2Page.tsx:589` |
|
|
| F4 | MED | 7 cờ ghi tuyệt đối ⇒ payload thiếu field = **tắt cờ im lặng** | `AdminFeatures.cs:424-440`, `:720-726` |
|
|
| F5 | MED | `DetachTrackedOpinionsOn` quét theo tên cột: bắt thừa + có thể nuốt `Added` | `AdminFeatures.cs:936-949` |
|
|
| F6 | LOW | `FullName ?? Email` ra null ⇒ 403 lòi dấu phẩy trống / liệt kê thiếu người | `PeWorkflowService.cs:709-714` |
|
|
| F7 | INFO | Lộ tên người duyệt: **không phải rò rỉ mới** (GET đã công khai tên+email) | `Controller.cs:21-26` |
|
|
| **F8** | **HIGH** | **2/3 file migration UNTRACKED ⇒ commit `-a` hạ toàn bộ 7 module** | `git status` |
|
|
| **F9-LOW** | LOW | Tên migration `…Pe…` gây hiểu nhầm (cột dùng chung ≥7 module) | migration filename |
|
|
| **F10a** | MED | Đổi **Mô tả** quy trình: ghi thẳng, không sinh vết changelog | `AdminFeatures.cs:698` |
|
|
| **F10b** | MED | Thêm người vào Cấp **đã qua** ⇒ màn hình hiện "Done" cho người chưa ký | `PeFeatures.cs:1135-1147` |
|
|
| **F10c** | LOW | Vết đổi phòng ban không ghi từ-đâu-sang-đâu | `AdminFeatures.cs:704` |
|
|
| **F11** | MED | Ca test `F16_…NoPeChangelog` **xanh giả** (0 phiếu PE ⇒ không phân biệt được) | `UpdateAwDefinitionTests.cs:753` |
|
|
| **F13a** | MED | Vá NỬA — `ContractWorkflowService.cs:264` vẫn in GUID trần | `ContractWorkflowService.cs:264` |
|
|
|
|
**TOTAL = 14 FLAG (4 HIGH · 6 MED · 3 LOW · 1 INFO).**
|
|
|
|
## Khuyến nghị deploy
|
|
|
|
Đánh giá **trên giả định F1/F2/F3 đã được vá xong** (wave 5), và lead tự xác minh lại bằng build + test + grep trước khi đẩy.
|
|
|
|
Ngoài F1/F2/F3, **còn đúng một thứ tao coi là chặn deploy: F8** — nó không phải lỗi mã nên vá mã bao nhiêu cũng không chạm tới nó, mà hậu quả thì nặng nhất bảng (mọi module dùng schema V2 ngừng duyệt). Nó chỉ tốn một lệnh `git add` đích danh cộng một lệnh kiểm chứng sau commit.
|
|
|
|
Các flag còn lại (F4, F5, F6, F10a, F10b, F11, F13a) **không chặn** đợt 1: F4 và F5 chỉ nổ qua đường PUT mà hiện chưa client nào gọi; F6/F10c/F13a là chất lượng thông điệp; F10b cần thao tác admin bất thường; F11 là nợ test chứ không phải lỗi chạy.
|
|
|
|
**DEPLOY-CÓ-RỦI-RO** — mã sẽ an toàn sau khi vá F1/F2/F3, nhưng rủi ro lớn nhất còn lại nằm ở **thao tác commit**: phải `git add` đích danh 2 file migration đang untracked (F8), nếu không prod sẽ có model chờ một cột mà DB không bao giờ được tạo.
|
|
|
|
<!-- END reviewer-diff-dot1 · TOTAL=14 FLAG -->
|