vietreport-admin/solution-erp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
e48a6c5068b9fb179b2417986b67634b787d1c0c
solution-erp/.claude/workflows/runs/2026-07-27-S155-pe-delete-approver/reviewer-diff-dot1.md
pqhuy1987 e48a6c5068 wal: flush 20260727T1211
2026-07-27 12:11:59 +07:00

20 KiB
Raw Blame History

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:

.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).

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

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

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.