# Taher WBA — Code & Architecture Critique **Date:** 2026-05-21 **Source:** `/Users/joedwy/dev/WebEntry` (the `WebEntry` repository), running locally against the April 3 production database snapshot. **Scope:** application architecture, service layer, Blazor frontend, async/concurrency, error handling, documentation, operability, testing, configuration & secrets, and general code hygiene. This is a companion to the separate **`db-architecture-assessment.md`**. Database/schema concerns are not repeated here. This is a blunt technical read, not a remediation plan. --- ## TL;DR The WBA codebase looks like what a competent two-person team builds under continuous feature pressure for several years with no architecture function, no testing function, no DBA, and no DevOps function. The code mostly works in production — but the structure has accumulated enough specific, identifiable problems that the cost of adding the *next* feature is meaningfully higher than the cost of the first. The dominant issues: 1. **God services with no interfaces.** `DataService` is 1,116 LOC; `DataEntryService` 813; `PlanService` 772. Concrete-class DI everywhere. No test seams. 2. **God components.** `SOPInvoiceEntry.razor` is 973 LOC; `SalesEntry.razor` 775; `WeekEntry.razor` 459. Pages mix UI, state, and direct DB access. 3. **Async/concurrency hygiene is unsafe.** `Task.Run(...).Result` blocking sync-over-async, `async void` background timers, `Parallel.ForEachAsync` mutating shared collections in a Blazor component. 4. **Two different Azure client secrets** committed to the repo in two different files. Plus a Cosmos DB primary key, plus SAGE ODBC credentials, plus the SQL SA password. 5. **No README, no architecture doc, no CI/CD pipeline, no health checks, near-zero tests.** Two `docs/` files exist and are good, but everything else has to be reverse-engineered. 6. **Pervasive silent error swallowing** — empty `catch { }` blocks, `catch { return null; }` without variable, `catch (Exception e) { }` where `e` is unused. 7. **A long tail of abandoned code in place** — 20+ blocks of commented-out logic that look like disabled features, plus dead projects (`Taher.PDFMockup` on .NET 7 while the main app is on .NET 8, `Taher.TestConsole` with no body, `Taher.Tests` with no tests). None of these are existential. All of them get more expensive to fix the longer they wait. Several have already caused at least one production outage somewhere — they just weren't recognized as architectural debt at the time. A grade, for calibration: **C / C-**. The architecture is recognizable as a real .NET 8 line-of-business app. There is real domain logic, real EF Core, real Blazor. But the discipline that turns "it works" into "we can change it safely" is largely absent. --- ## 1. Architecture & Layering ### 1.1 Project structure: ten projects, weak boundaries The solution has thirteen projects: | Project | Purpose | Health | |---|---|---| | `Taher.WebEntry` | Blazor Server host | active, monolithic | | `Taher.Data` | EF Core context, models, SQL artifacts, migrations | active | | `Taher.Service` | Application services | active, dominated by god services | | `Taher.BusinessLogic` | Pure calculation utilities | active, untested | | `Taher.Hangfire` | Hangfire wiring + dashboard | active | | `Taher.Reporting` | Report models & caching | active | | `Taher.Logging` | Cosmos-backed logging | active | | `Taher.Common` | Enums & extension methods | active | | `Taher.Model` | Service-layer DTOs / view-model types | active | | `Taher.Tests` | xUnit project | **empty — only a `.csproj`** | | `Taher.X.Tests` | xUnit project | **2 tests, both for `SalesItem` revenue math** | | `Taher.TestConsole` | Console scratchpad | **empty `Program.cs`** | | `Taher.PDFMockup` | Standalone PDF console app | **targets .NET 7; not referenced anywhere** | | `Taher.Jobs` | PowerShell sync script | one file, undocumented | The boundaries between `Taher.Data` (EF + models) and `Taher.Model` (DTOs) are not crisp. `UnitTenantService` — the multi-tenant gating service — lives at `Taher.Data/Model/UnitTenantService.cs` but is declared in the `Taher.Model` namespace. Other classes do the reverse. There is no rule, just history. `Taher.PDFMockup` targets `net7.0` (`Taher.PDFMockup/Taher.PDFMockup.csproj:3`), while every other project targets `net8.0`. Nothing in the active build path references it. Either it's a forgotten artifact or there's an offline PDF-generation flow no one documented. ### 1.2 Service layer: kitchen-sink classes with no interfaces `Taher.Service/DataService.cs:15-37` accepts six other services in its constructor — `DataEntryService`, `PlanService`, `InvoiceService`, `UnitService`, `CommentService`, `SharepointService`, `ReportService` — most of which it never uses; they're listed for unclear reasons (one parameter, `_dataEntryService`, is stored; the others are not). The class then exposes 46+ public methods spanning unit loading, period entry updates, grocery purchases, line-item add/remove, milk cost updates, period sales sync, Sage timestamp updates, and inventory record updates. There is no thematic split. `DataEntryService.cs` (813 LOC) and `PlanService.cs` (772 LOC) follow the same pattern. There is **no `IDataService`, no `IPlanService`, no `IInvoiceService`** in the codebase. `Program.cs:102-114` registers everything by concrete type. The `docs/testing-concerns.md` document calls this out; nothing has changed since. Practical consequences: - A Blazor component cannot be unit-tested without a live SQL Server, because the only way to fake `DataService` is to subclass it, but its methods are not virtual. - The compiler can't help refactors — there is no contract to honor, just a 1,100-line surface. - Service-on-service calls form an implicit graph that is invisible until you walk the constructors. ### 1.3 EF Core ↔ stored procedure boundary: no policy `Taher.Data/SQL/` contains 15 stored procedures and 14 table-valued functions, with their own home-grown versioning scheme (`sp/CreateMissingPeriodEntries/v1.sql` through `v14.sql`). They are invoked from C# via `Database.ExecuteSql` from at least 13 call sites across `PlanService`, `DataEntryService`, and `TaherContext.OnSave.InvalidateCachedReports.cs`. The split between SQL and C# has no documented rule. Heavy set-based work (period creation, plan sync, declining balance) lives in SQL; CRUD lives in C#. But declining-balance math also exists in C# (commented out in `TaherContext.OnSave.cs:309-324` — see §11). Some sprocs are called every save; some are called from background jobs. Some are called from migrations; most are not. The versioning is also redundant with EF's `migrationBuilder.Sql(...)` + embedded resources, but it persists because nobody has decided whether the SQL layer is "EF-managed" or "manually managed." It is currently both. ### 1.4 Background work: timer-driven `IHostedService` + Hangfire, no separation of concerns `Taher.Service/Workers/ADSyncService.cs` is registered as a hosted service. It runs every ~10 minutes via `System.Threading.Timer`. The timer callback is **`async void JobProcessor(object? state)`** (`ADSyncService.cs:58`). The class tracks `_busy` manually to prevent re-entrance. Exceptions in this callback are unobservable. If a sync hangs for 15 minutes, the next tick fires anyway. Hangfire is *also* present (`HangfireJobsHelper.cs:11`) and runs the Clover and ADP sync jobs on its own schedule. The two systems coexist with no documentation of why some background work uses one and some uses the other. The Hangfire dashboard authorization is a hardcoded email allowlist: ```csharp var hangfireUsers = new List { "k.grundy@Taher.com", "m.kumar@Taher.com", "w.balol@Taher.com" }; ``` (`Taher.Hangfire/Helpers/DashboardHelper.cs:23`.) Already flagged in security-concerns.md; still in production. ### 1.5 Multi-tenant model: opt-in, not opt-out `TaherContext.Tenant.cs:7-43` installs EF query filters on **9 entity types** out of ~89: `BusinessUnit`, `Building`, `Employee`, `BudgetPlan`, `PeriodLabor`, `PeriodSalesItem`, `PeriodCostItem`, `InventoryRecord`, `PeriodAdpLabor`. The other 80 entities — `Invoice`, `InvoiceLineItem`, `Comments`, `GLCodes`, `BudgetPlanCost`, `BudgetPlanRevenue`, `BudgetPlanLabor`, `EmployeeRate`, `ServiceTickets`, `_AuditEntries`, all the `OpsRep*` tables — have no row-level tenant filter at all. Worse: `DataService.FindAsync` defaults `ignoreQueryFilters: true` (`DataService.cs:236-238`). The most natural single-entity-by-ID lookup pattern silently bypasses every filter. **Tenant isolation is opt-in, not opt-out.** Throughout the codebase there are 40 explicit `IgnoreQueryFilters()` call sites — many of them in places that don't obviously need it (e.g., loading vendors, loading cost items inside a declining-balance summary). The total picture is: tenant isolation is enforced by service-code discipline, not by the database or by the query layer. ### 1.6 DbContext lifetime: Transient + Scoped factory `Program.cs:153-170` registers **both** `AddDbContext(...)` with `ServiceLifetime.Transient` *and* `AddDbContextFactory(...)` with `ServiceLifetime.Scoped`. The Transient choice on the context itself is unusual — most ASP.NET Core apps register the context as Scoped so it shares a unit of work within a request. Transient + Factory simultaneously means consumers can resolve a fresh context (Transient injection) *or* pull one from the factory (Scoped factory). There is no apparent rule for which to use. Several Blazor components use the factory correctly (`using var context = DbContextFactory.CreateDbContext()` — `Index.razor:52`). Some inject `TaherContext` directly through services that hold a constructor reference. Connection pool churn is the visible cost; the deeper cost is unpredictable change-tracker scopes. ### 1.7 Auto-migration on startup `Program.cs:201` calls `ApplyMigrations()` synchronously during app startup, with a 30-minute command timeout (`Program.cs:243`). Already flagged in security-concerns.md, but worth restating because every other architectural decision in this app is downstream of it: a deployment with a broken migration is also a deployment with a non-starting application, which is also a deployment with no rollback path. --- ## 2. Frontend (Blazor Server) ### 2.1 Component file size | Component | LOC | Notes | |---|---:|---| | `Pages/Invoice/SOPInvoiceEntry.razor` | 973 | invoice entry + duplicate detection + validation | | `Pages/WeeklyEntry/FlashItems/SalesEntry.razor` | 775 | mixes nested LINQ pivot rendering with budgeting calculations | | `Pages/WeeklyEntry/DCR/DCRMonthView.razor` | 565 | | | `Pages/WeeklyEntry/DailyCashRegisterEntry.razor` | 554 | | | `Pages/WeeklyEntry/DCR/DCRInfoEntry.razor` | 490 | | | `Pages/WeeklyEntry/WeekEntry.razor` | 459 | cascades `this` to children to expose internal methods | | `Pages/WeeklyEntry/FlashItems/MonthlySummaryDisplay.razor` | 413 | | | `Pages/WeeklyEntry/FlashItems/LaborEntry.razor` | 366 | | | `Pages/WeeklyEntry/FlashItems/InventoryEntry.razor` | 316 | | Several of these compose **inline LINQ pivots inside markup**. `SalesEntry.razor:132-256` runs `_revenue.GroupBy(...).OrderBy(...).ThenBy(...).GroupBy(...)` in a `@foreach` that generates the table directly, with per-day conditional rendering of budget/plan/actual columns. There is no precomputed view model; the markup *is* the query plan. Performance profiling and reuse become impossible. ### 2.2 Anti-patterns in component lifecycle - **Busy-wait poll in render:** `Index.razor:64-72`: ```csharp while(_units is null) await Task.Delay(10); ``` This loops on the SignalR render thread waiting for `OnParametersSetAsync` to finish. A `TaskCompletionSource` would do the right thing. - **Imperative `StateHasChanged()` after every mutation:** `SOPInvoiceEntry.razor:707, 733, 750, 771` — four explicit calls in the same file, each after an inline state mutation. A component that's binding properly does not need to manually re-render. - **Cascade `this`:** `WeekEntry.razor:19` cascades the component instance as a `[CascadingValue]` so child components can reach back and invoke internal methods (`_entryAuditPage.SetItems`). Children mutating parents through a back-channel breaks encapsulation and obscures data flow. ### 2.3 Concurrency inside a component `SalesEntry.SetBudgetValues()` (`SalesEntry.razor:560-599`) runs `Parallel.ForEachAsync` over a budget set, with a nested `Parallel.ForEach` inside, mutating a `ConcurrentBag` plus the rendered `_revenue` items. There is no `CancellationToken`, no error handling, and no synchronization with the render loop. Blazor Server has a single-threaded sync context per circuit, and this hops off it deliberately. If one iteration fails the rest continue silently. In a SignalR-heavy environment with reconnects this is a race waiting to be a bug report. ### 2.4 JS interop hygiene The `_Layout.cshtml` script-load-order race I already noted is one symptom; the more general problem is that JS interop is everywhere with no shared module structure: - `JSRuntime.InvokeVoidAsync` and `InvokeAsync` are called from ~40 component call sites. - `MainLayout.razor:80` calls `registerInteractionDetection` without guarding for `JSDisconnectedException` (which we saw fire when the home-page Blazor circuit died — `webentry.log` evidence from the earlier session). - `wwwroot/js/taher-app.js` is a global class with static methods (`Modal`, `Fade`, `CrossFade`, `DownloadCSV`, `DownloadPDF`) that depend on jQuery, Bootstrap 5 modal, and SweetAlert2. `interaction.js` declares a bare top-level function. There is no module system, no namespacing beyond the global object. - `package.json` exists and contains exactly one dependency: **jQuery 3.6.3**. In a .NET 8 Blazor Server app. ### 2.5 Loading and error UX is inconsistent Some components show a `LoadingSpinner` while `_units is null`; some comment out the spinner entirely (`MonthlySummaryDisplay.razor:18` is a commented-out loading guard); some have no spinner at all and just render an empty grid. The lone `ErrorBoundary` lives in `MainLayout.razor:18-36` and renders a generic `` content for everything. There is no toast pattern, no inline form-level error, no validation summary discipline. A hung query produces a permanent spinner. ### 2.6 Styling - `wwwroot/css/taher-corporate.min.css` is ~158k tokens of minified rules. - Three `*.razor.css` scoped CSS files exist (out of 100+ components). - Inline `style="..."` attributes appear in 20+ components for layout fixes — e.g., `style="height: 40px; border-left: 1px solid #dde0e5;"` in `WeekSelectorControl`. - No design tokens. `bg-taherblue` is a hardcoded utility class. - Accessibility is essentially absent: ~65 ARIA attributes across the entire frontend, no `aria-live` for form errors, manual `TabIndex="GetTabIndex()"` counter management instead of semantic focus order. ### 2.7 Newtonsoft.Json vs `System.Text.Json` `Index.razor:2, 100, 109` uses `Newtonsoft.Json` to round-trip a small UI-state object through `localStorage`. Same pattern in `SalesEntry.razor`. Elsewhere the app uses `System.Text.Json` (in `LocalStorageService.cs`, `DevOpsService.cs`, etc.). `Taher.Data/TaherContext.OnSave.cs:150-151` uses `Newtonsoft.JsonConvert` to serialize audit payloads without specifying any settings — no reference-loop handling, no null-value policy. There is no rule. The two libraries coexist by accident. --- ## 3. Async, Concurrency & Threading ### 3.1 Sync-over-async (`.Result`) in request paths - `UnitTenantService.GetServiceResult()` at `Taher.Data/Model/UnitTenantService.cs:73-77` does: ```csharp var membershipTask = Task.Run(async () => await GetMembershipsAsync()); var accountingTask = Task.Run(async () => await GetAccountingUsersAsync()); return membershipTask.Result; ``` This is a classic deadlock recipe in ASP.NET Core. It's called from the `TaherContext` constructor (line 15), which means **every DbContext instantiation** can block a thread on Microsoft Graph round-trips. The `accountingTask` is started and then discarded — fire-and-forget without observation. - `LogService.LogError` (in `Taher.Logging/LogService.cs:34-45`) does the same pattern to extract the authenticated user's email synchronously inside what should be an async logging path. ### 3.2 `async void` and fire-and-forget - `ADSyncService.JobProcessor` (`ADSyncService.cs:58`) — the 10-minute Graph sync — is `async void` on a `System.Threading.Timer` callback. Exceptions are uncatchable. - `ReportCachingService.SaveCachedReportAsync` is declared `async` and returns `Task`, but its body is just `await Task.CompletedTask;` after a synchronous call. The async signature is decorative. - The `accountingTask = Task.Run(...)` in `UnitTenantService` above is started and never awaited. ### 3.3 `Parallel.ForEachAsync` in Blazor components `SalesEntry.razor:560-599` — already noted in §2.3. Parallel work mutating shared collections rendered by Blazor's single-threaded sync context. The fact that this currently works in production is evidence the team has been lucky, not that the code is safe. ### 3.4 Concurrency in `TaherContext.OnSave` `TaherContext.OnSave.cs:43-48`: ```csharp catch(SqlException ex) when (ex.Number == 2627) { //ignore PK issue with report caching trySave++; } ``` Catching a primary-key violation and silently retrying tells you the team knows there's a race between report-cache writers but treats the symptom rather than naming it. --- ## 4. Error Handling ### 4.1 Silent swallows Three notable examples, all in `Taher.Data/Model/UnitTenantService.cs`: ```csharp // line 346-349: catch (Exception ex) { } // GetAccountingUsersAsync — variable declared, never used // line 354-357 / 378-381: catch { return null; } // GetAuthenticatedUserId / GetAuthenticatedUserEmail — exception lost ``` And in `Taher.Logging/LogService.cs:66-69`: ```csharp catch { // intentionally ignored } ``` The first one breaks AD group sync silently when Graph fails. The second hides identity errors. The third loses the logging system's own failures. All three are *worse than no try/catch* — they convert hard failures into invisible degradations. ### 4.2 Catch-log-rethrow pattern is good but context is thin Most service methods follow `catch (Exception e) { await LogErrorAsync(...); throw; }` (e.g., `DataService.cs:64-77`). The intent is right, but the logged context is whatever the method captures locally — usually just the entity ID. In a batch loop, you lose which iteration failed. ### 4.3 No domain exception types Bare `throw new Exception("...")` is the universal pattern. `CloverService.cs` throws "Clover unit not found for unit {n}" as a plain `Exception`. There are no `NotFoundException`, `ConcurrencyException`, `ValidationException` — callers cannot react differently to transient vs. permanent vs. invariant failures. ### 4.4 Null-coalescing as silent error suppression `DataService.cs:42`: ```csharp return authenticationState.User?.Identity?.Name ?? string.Empty; ``` Returns `""` when auth is broken. Callers using this as the "audit user" string have no way to know they're recording untraceable changes. --- ## 5. Secrets & Configuration This deserves its own section because the leakage is broader than `docs/security-concerns.md` acknowledges. ### 5.1 Two different Azure client secrets in the repo - `Taher.WebEntry/Properties/launchSettings.json:10` — `AZURE_CLIENT_SECRET = "zlp8Q~mTWqdYgaTdWn-GZZOhO~dwqUFmD6TyXcdT"` - `shell.nix:25` — `AZURE_CLIENT_SECRET = "OUt8Q~4ZwYm4nQ5M-_om~9Z-MOdNmRTC36zPuay8"` These are *different* values. Either both are valid (worst case), or one was rotated and the old one was never scrubbed (most likely). Either way, the repository now contains two credentials that grant access to the production Taher Azure AD tenant, both committed to git history. ### 5.2 Cosmos DB primary key, hardcoded `Taher.Logging/CosmosLoggingContext.cs:11`: ```csharp optionsBuilder.UseCosmos( "https://taher-global-logging.documents.azure.com:443/", "F727Gznd8fkTP1loGUHzVcWT50RWEudB8IIthKcr4wON3FFdQ3bTcy1qBD6eQwRxEUArF8m53oOzACDbAEj0fg==", databaseName: "TaherGlobalLogs"); ``` This is the **master key for the Cosmos account that hosts the logging database**. Anyone with read access to the repo can read or modify all application logs. ### 5.3 SAGE ODBC credentials in PowerShell `Taher.Jobs/WebEntrySync.ps1:6-11`: ```powershell $connStr = @" DSN=SOTAMAS90-TAH; UID=cp; PWD=Jazzband82; "@ ``` Already in security-concerns.md. ### 5.4 Shared `APIHashBase` The Sage integration uses a SHA256 HMAC with a shared secret (`30s60t3ayypgyenx`) hardcoded in `WebEntrySync.ps1` and matched in `SageIntegrationController`. Already in security-concerns.md. ### 5.5 SQL SA password in `docker-compose.yml` Already in security-concerns.md. Mitigated for local dev by my `docker-compose.local.yml` reading from `.env`. ### 5.6 Config sprawl Configuration is split across: - `appsettings.json` (committed — contains the App Insights key, AAD tenant/client IDs, Key Vault URI) - `appsettings.Production.json` (committed) - `appsettings.Local.json` (mine — local-only, git-ignored) - `launchSettings.json` (committed, contains the secret) - `shell.nix` (committed, contains the other secret) - `Taher.Logging/CosmosLoggingContext.cs` (committed, contains the Cosmos key) - environment variables (undocumented) `Program.cs:30-35` loads Key Vault if a URI is configured, but no validation that required secrets actually loaded. If Key Vault is unreachable, the app starts up with empty secrets and unpredictable runtime behavior. **Net:** there are at least four production credentials in this repo today, plus history. A real cleanup is "rotate everything, scrub history with `git filter-repo`, move to Key Vault, validate-on-startup." None of that has happened. --- ## 6. Documentation ### 6.1 What exists - `WebEntry/docs/security-concerns.md` — well-written, accurate - `WebEntry/docs/testing-concerns.md` — well-written, accurate - That's it. ### 6.2 What doesn't exist - **No root README** — no domain explanation, no setup instructions, no architecture overview, no diagrams, no onboarding guide. - **No XML doc comments** on most public APIs. Sampled `DataService.cs:15-37`: no `` on the constructor or any of its 46+ public methods. `Taher.Service` has 8 classes with summaries out of ~22 public classes. `Taher.BusinessLogic` has 1 of 8. - **No ER diagram**, no entity relationship documentation, no description of the period-record/budget-plan effective-date model — which is the conceptual core of the entire app. - **No runbook**. If the Hangfire server stops processing the Clover sync, there is nothing in the repo that tells you which logs to check or how to manually run a missing window. - **No deployment doc**. The app is in Azure App Service; nothing in the repo says how it gets there. - **No API documentation** beyond `AddSwaggerGen()`. The `/swagger` endpoint serves a near-empty OpenAPI doc because none of the controllers have `[ProducesResponseType]`, ``, or `[ApiController]` decoration that produces useful Swagger output. ### 6.3 Inline comments Inline comments mostly explain *what* the code is doing, not *why*. Five TODO/FIXME tags total across the whole codebase: - Three are boilerplate from the `IDisposable` template (`FlashModel.cs`). - One marks unfinished labor-cost feature work: `Taher.Reporting/ReportModels/FLASH/FlashModel.BAndI.cs:1` — `// TODO labor here`. Vague single-word note. - One marks `CloverService.cs:50` — "TODO: move hardcoded URLs to config." Still hardcoded. Two large commented-out blocks in `TaherContext.OnSave.cs` (lines 195-198 and 213-237) carry no explanation. They look like a disabled feature, not dead code. ### 6.4 What this implies A new engineer's onboarding path is: clone the repo, get nothing from it about what the system *does*, learn the domain by reading screens, learn the architecture by reading code, ask a senior engineer for the rest. That works at a 3-person team. It does not work at scale. --- ## 7. Observability & Operability ### 7.1 No health endpoints No `/health`, `/health/live`, `/health/ready`. Azure App Service health-check probing will succeed as long as the process is up, regardless of whether the database connection is alive, Key Vault is reachable, or Hangfire is processing. ### 7.2 Two logging systems running in parallel - `Application Insights` is wired in `Program.cs:191` with the instrumentation key in `appsettings.json:9`. No custom telemetry, no metrics, no dependency tracking — basically default ingestion. - `Taher.Logging.LogService` writes to a custom Cosmos DB container (`CosmosLoggingContext`) with a hardcoded primary key. Catches its own failures silently (`LogService.cs:66`). Constructed fresh per call — no pooling. The two logging paths share no correlation. There is no request ID, no trace context propagation, no distributed tracing. Investigating a failure means cross-referencing App Insights timestamps with Cosmos log rows by hand. ### 7.3 Migration story is also a runbook gap 159 migrations applied; `__EFMigrationsHistory` is the only authoritative record of "what's in the DB." No documentation says which migration corresponds to which release. If a migration fails on startup, the app does not boot. There is no documented rollback procedure. ### 7.4 No CI/CD pipeline No `.github/workflows`, no `azure-pipelines.yml`, no Docker-based build target for the app. The only Dockerfile in the repo (`WebEntry/Dockerfile`) is a *SQL Server image* augmented with sqlpackage — it builds the database tooling container, not the application container. This means: - "Does the latest commit build?" can only be answered by checking out and running `dotnet build`. - The 416-warning build output is never gated. - Tests (such as they are) only run if someone remembers. - Deployments happen out-of-band — likely from Visual Studio or a developer's machine. ### 7.5 Hangfire dashboard is operationally important and badly guarded The Hangfire dashboard is the only built-in operability surface — it shows job status, retries, queue depth. It is gated by the three-email hardcoded allowlist. If someone leaves Taher, you need a code deploy to revoke their access. If someone needs temporary access, you need a code deploy to grant it. --- ## 8. Testing Already covered in `docs/testing-concerns.md`. Summary, for completeness: - `Taher.Tests/` is empty (only the `.csproj`). - `Taher.X.Tests/` has 2 tests, both for `SalesItem` revenue math. - `Taher.TestConsole/Program.cs` is an empty `// See https://aka.ms/new-console-template…` template. - No integration tests, no controller tests, no service tests, no Blazor component tests. - No CI to run any of it. - No interfaces, so even if someone wanted to add tests, half the work would be extracting seams first. The combination of "no interfaces" + "no tests" + "god services" + "auto-migration on startup" is the core safety problem. Each one makes the others harder to fix. --- ## 9. Code-Quality Smells (catalog) A non-exhaustive list of patterns that recur. Each is small in isolation; together they communicate the state of the discipline. 1. **416 compiler warnings on a clean build of `Taher.WebEntry`.** Mostly nullability (`CS8602`, `CS8604`, `CS8618`). Some are real null-deref risks. Nobody triages this output. 2. **Obsolete API usage:** `Microsoft.Graph.BatchRequestContent` / `AddBatchRequestStepAsync` — six `CS0618` warnings in `UnitTenantService.cs`. SDK is one major version behind the deprecation. 3. **Mixed Newtonsoft and System.Text.Json** — see §2.7. 4. **`async void` on event handlers** — that's fine. **`async void` on a `Timer` callback** — see §3.2. 5. **String-interpolated raw SQL** in places — even though `Database.ExecuteSql` is FormattableString-aware, some calls pass concatenated strings (`PlanService.cs:370`). 6. **Bare `catch { … }` blocks** — see §4.1. 7. **`!` null-forgiving operators sprinkled to silence warnings** rather than to assert real invariants. 8. **Magic strings.** AD group prefixes (`"WebEntry - Admin"`, `"Unit - "`) are constants in `UnitTenantService` but the GUID `"2b9579a6-4f04-40e2-b9e6-70b58cc753ca"` (the "all units" group) is a hardcoded string literal. 9. **Inconsistent async method naming.** Most are `*Async`; a few aren't. 10. **`FluentValidation` is referenced in `Taher.WebEntry.csproj` but no validator implementations exist** anywhere. The package is a dead dependency. 11. **`a.pdf`, a 43 KB binary**, is checked into `Taher.WebEntry/a.pdf`. Likely an accidental commit no one noticed. 12. **`package.json`** in a .NET repo contains exactly one dependency (`jquery: ^3.6.3`) and nothing else — no scripts, no dev-deps, no lockfile. --- ## 10. Dead Code & Abandoned Refactors A specific category of smell, worth its own list because the pattern reveals process more than craft. 1. `TaherContext.OnSave.cs:213-237` — `UpdateBudgetPlanEffectiveDateEnd` is ~21 lines of *entirely commented-out* code. The method exists and is called from `EntityConstraints`, but does nothing. The original logic — maintaining `EffectiveDateEnd` boundaries on budget plans when a new plan is created — is disabled. No comment says why. 2. `TaherContext.OnSave.cs:309-324` — `UpdateDecliningBalanceCachedTable` is entirely commented out. Called twice from `SaveChanges` / `SaveChangesAsync`. It looks like a feature that was disabled at some point. 3. `Taher.PDFMockup` — entire project on .NET 7, never referenced by the active build. Probably abandoned. 4. `Taher.TestConsole` — empty `Program.cs`, never invoked. 5. `Taher.Tests` — `.csproj` only, no source files. 6. The audit skip-list bug in `TaherContext.OnSave.cs:89` (`entry.Metadata.GetType() == typeof(AuditEntry)` — already covered in the DB assessment) is also dead code in a different sense: the intended behavior never executes. 7. `Microsoft.AspNetCore.Authentication.AzureAD.UI` 6.0.33 is referenced in `Taher.WebEntry.csproj:41` — but the project has migrated to `Microsoft.Identity.Web 3.0.1`. The older AzureAD.UI package is *deprecated*; Microsoft has retired it. It is in the manifest because nobody removed it. 8. Inline comments referencing a `connectionstring` that has since changed (e.g., a vestigial dev SQL connection string in `appsettings.json:30`). The pattern across all of these: features were disabled or projects were stood up and then nothing was cleaned up. There is no end-of-cycle hygiene. --- ## 11. Migrations Story 159 EF migrations, forward-only, from `20230426144226_InitialSchema` through `20260216111720_AddFixedMonthlyCostDateRange`. ~3 years of cadence. The history is *clean* — no rewrites, no manual rollbacks, no `Designer.cs` corruption — and that's a real positive (see §13). What's less clean: - 95 of the 159 migrations mutate existing tables (add column / rename / drop column). High edit ratio. - 34 create or drop tables — new domains are still being added (PeriodAdpLabor, UnitPayrollRelatedConfig, VendorInvoiceRelationship in 2025/2026). These new tables inherit the same `FLOAT`-for-money / `nvarchar(max)` defaults as the old ones — see the DB assessment. - 200 create indexes. No consolidation migrations exist; index growth is monotonic. The shape of the history suggests a steady feature pace with no time set aside for schema hygiene. --- ## 12. Integration Code A quick walk through `Taher.Service/CloverService.cs`, `AdpService.cs`, `SharepointService.cs`, `SageIntegrationController.cs`: - **CloverService**: has a Polly retry policy for HTTP calls (line ~39 — one of the only retry policies in the codebase). Logs errors to Hangfire console and `LogService`. Throws bare `Exception` ("Clover unit not found"). Some hardcoded URLs flagged for `// TODO: move to config`. - **AdpService**: SFTP credentials hardcoded (already in security-concerns.md). No retry. Catches Exception and continues. - **SharepointService**: contains a method `GetDriveId()` (line ~25) that builds a `GraphServiceClient` and never stores or returns the result — dead method body. - **SageIntegrationController**: `[AllowAnonymous]` with HMAC-of-timestamp auth, no replay protection (already in security-concerns.md). The pattern is that integrations were added one at a time, each authored by whoever happened to be free, with no shared retry/error-handling/secrets-loading convention. --- ## 13. Things That Are Done Well To be fair, this isn't all bad. - **Migration discipline.** 159 forward-only migrations across 3 years, no rewrites — that's real. - **Soft-delete pattern.** Consistent `IsDeleted` flag, consistent `(IsDeleted, Id)` indexes, consistent `Delete()` method on the base entity. Even though §1.5 critiques the tenant gap, the soft-delete pattern itself is coherent. - **EF query filters are used,** even partially. Many .NET LOB apps skip them entirely. - **`AsSplitQuery()`** is correctly used in a few places where multiple `Include` chains would otherwise Cartesian-explode. - **`docs/security-concerns.md` and `docs/testing-concerns.md` exist and are honest.** Someone wrote them down. That's already further than most teams get. - **`Polly` retry** in `CloverService` shows at least one engineer knows the right shape. - **The `BaseSpecification` query pattern** in `Taher.Data/Query/` is a real attempt at a reusable query shape. - **The Business Logic project (`Taher.BusinessLogic`)** is mostly pure static functions — tax splitting, fringe rates, fiscal week generation. The fact that these are isolated is good architecture; the fact that they're untested is unfortunate. --- ## 14. What This Says About Engineering Process A schema is a fossil record. So is a codebase. What this one preserves: - **Features are prioritized over hygiene.** Comments are about what, not why; abandoned features stay in place; dead projects stay in solution; warnings accumulate. - **No test-first culture, ever.** It's not "tests were missed"; the architecture *prevents* unit testing (no interfaces, god services, direct DB injection in components). That's a design choice repeated for years. - **No DBA role and no architecture role.** The same person owns the schema, the service, the UI, the integration, and the migration. That works until it doesn't. - **No DevOps role.** No CI, no health checks, no correlation, two parallel logging systems, four committed secrets, undocumented deployment. - **No code review discipline.** Two different Azure client secrets sitting side-by-side in two committed files is the kind of thing a peer reviewer catches in the first week. - **There has been at least one near-rotation of credentials** (two AAD secrets in the repo) but no scrub of the old one — suggesting the rotation was done in the portal and the team wasn't sure how to scrub git history. The honest summary is that the team behind WBA is a competent feature team that has been given more business pressure than architectural runway, for long enough that the architecture is now a constraint on the business pressure. The system isn't dying — it's working. But the cost of adding the next feature is meaningfully higher than the cost of the last one, and that gap will keep widening until someone is given time to invest specifically in the code's structural health. --- ## 15. Risk Ranking Ranked by approximate cost-to-business if left alone. 1. **Two committed Azure client secrets + Cosmos primary key + SAGE creds.** Credential rotation that didn't scrub history. Anyone with repo read access has these. Material security exposure, today. 2. **Sync-over-async in `UnitTenantService.GetServiceResult`** called from `TaherContext` constructor. Latent deadlock under load. Has probably already caused at least one mysterious "the app is slow" incident. 3. **God services with no interfaces.** Blocks all testing. Compounds every other problem because no one can verify a fix. 4. **Partial tenant filtering + `FindAsync` default of `ignoreQueryFilters: true`.** Real authorization gap; one routing or controller bug from a data-leak incident. 5. **No CI / no tests / no health checks.** "Did the last commit break production?" can only be answered by trying production. 6. **Auto-migration on startup with 30-minute timeout.** A bad migration is also an outage. 7. **`async void` Timer in `ADSyncService`.** Silent failure mode for a recurring sync. If Microsoft Graph throttles, nobody notices until "the AD groups look stale." 8. **`Parallel.ForEachAsync` mutating Blazor component state.** Race in production every time it fires; usually wins. 9. **Hangfire dashboard's three-email allowlist.** Operational fragility. 10. **Two parallel logging systems with no correlation.** Slows every investigation by an order of magnitude. 11. **No README / no XML docs / no runbook.** Onboarding penalty, knowledge concentration risk. 12. **Dead code in `OnSave.cs` (commented budget logic, commented declining-balance cache).** Latent correctness risk — someone might rely on a feature that's silently disabled. 13. **Mixed Newtonsoft + System.Text.Json.** Annoyance more than risk. 14. **416 warnings on every build.** Slow erosion of signal. 15. **`a.pdf` in source control / dead `Taher.PDFMockup` project.** Cosmetic. --- ## Closing The WBA codebase reflects a normal trajectory for a feature-driven product team without dedicated architecture, testing, or operability investment. The shipped product works; the long tail of small smells around it is what you'd expect after 3+ years of "ship the thing the business asked for." The most worrying items are not architectural — they're operational. Multiple production credentials sit in git history. There is no CI to know whether the next commit even builds. There is no way to verify a change without trying it on the user. There are no health probes to know whether the app is alive in any meaningful sense. The next person to own this codebase will spend their first three months in the dark — there is nothing written down. That's solvable. But it's solvable only if someone is *given* the time to solve it; if the answer to every quarter's planning is "more features," the gap widens.