# Unanswered Questions This file collects questions that cannot be answered with best practices alone and require user decision. --- ## Best Practice Decisions (Already Applied) These common questions have been pre-answered with best practices: | Question | Decision | Rationale | |----------|----------|-----------| | Async vs sync methods | Async-first with CancellationToken | Modern .NET pattern, better scalability | | Exception handling | Custom typed exceptions per layer | Clear error propagation, testability | | Configuration | IOptions pattern | Type-safe, hot-reload support | | Logging | ILogger with structured logging | Framework integration, filtering | | Testing | xUnit + Shouldly + NSubstitute | Project constraints (no FluentAssertions) | | DI lifetime | Scoped for DB, Singleton for config | Standard patterns | | Nullable refs | Enable project-wide | Modern C# safety | | Oracle driver | Oracle.ManagedDataAccess.Core | Single driver for JDE and CMS | | Excel library | ClosedXML (MIT) | Free, replaces EPPlus | | Date types | DATETIME2(7) | Better precision | --- ## Phase-Specific Questions Questions below require user input before implementation. --- ### Phase 2: setup-solution-foundation **Q2.1: Cron schedule disable pattern** - Context: Design uses empty cron strings to disable schedules in Development - Options: (a) Empty string = disabled, (b) Add `Enabled` flag, (c) Use `string?` with null = disabled - Recommendation: Use `Enabled` flag for clarity - Impact: DataSyncOptions, schedule evaluation logic **Q2.2: Temp directory portability** - Context: ExcelExportOptions defaults to `/tmp/lotfinder` which is Linux-only - Options: (a) Use `Path.GetTempPath()`, (b) Make required config, (c) Platform-detect at runtime - Recommendation: `Path.Combine(Path.GetTempPath(), "lotfinder")` as default - Impact: Excel export temp file handling --- ### Phase 3: implement-domain-models **Q3.1: Requirement count discrepancy** - Context: Tasks claim 52 requirements but base spec has 34 sections - Options: (a) Count includes delta spec additions, (b) Update to correct count - Recommendation: Update to accurate count from base spec - Impact: Acceptance criteria verification **Q3.2: Namespace convention** - Context: Base spec references `ScopingTool.Domain.Models`, tasks use `JdeScoping.Core/Models` - Options: (a) Use JdeScoping.Core (matches solution), (b) Use ScopingTool.Domain (matches spec) - Recommendation: Use `JdeScoping.Core.Models` (matches existing solution structure) - Impact: All entity file locations, using statements --- ### Phase 4: implement-data-access **Q4.1: Repository method counts** - Context: Tasks list wrong method counts (17/18) vs spec (23/22) - Options: Update task counts to match spec - Recommendation: Fix counts during implementation - Impact: Task list accuracy --- ### Phase 5: implement-data-sync **Q5.1: JDE/CMS fetcher deferral** - Context: Proposal defers real JDE/CMS connectivity and CMS circuit breaker - Options: (a) Implement real fetchers now, (b) Use mock fetchers initially, (c) Implement core service with stub fetchers - Recommendation: Implement core service pattern with interface abstraction, defer real Oracle/Sybase connectivity - Impact: Testing approach, deployment timeline --- ### Phase 6: implement-search-processing **Q6.1: Stored Procedure for Downstream Traversal** - Context: Legacy system generates WHILE loop inline in T4 template for downstream work order traversal (up to 20 iterations). Base spec suggests stored procedure. - Options: (a) Create new stored procedure `dbo.TraverseWorkOrders`, (b) Keep inline WHILE loop in SqlKata-generated SQL, (c) Use C# iterative approach with multiple queries - Recommendation: Option (a) - stored procedure reduces network round trips and maintains transaction consistency - Impact: Requires adding stored procedure to Phase 1 (migrate-database-schema) if not already present **Q6.2: MIS Extraction Query Strategy** - Context: MIS data extraction queries do NOT join `#Temp_WO` per Codex review findings. MIS extraction is somewhat independent of main search. - Options: (a) Keep MIS extraction as separate SqlKata queries, (b) Create dedicated stored procedure for MIS extraction, (c) Separate MIS extraction into its own service - Recommendation: Option (a) - separate SqlKata queries via `MisQueryBuilder` class - Impact: Affects `ISearchQueryBuilder` interface design **Q6.3: Filter Entry Location** - Context: Legacy filter entry classes are in `OLD/WorkerService/Models/Reporting/`. Design places them in `JdeScoping.SearchProcessing/Models/FilterEntries/`. - Options: (a) Place in SearchProcessing project, (b) Place in Domain Models project, (c) Create separate JdeScoping.Shared project - Recommendation: Option (a) - filter entries are specific to search processing - Impact: Project references and potential code duplication **Q6.4: Streaming vs Materialization Default** - Context: Design supports both `IAsyncEnumerable` streaming and `Task` materialization. - Options: (a) Primary API returns streaming, caller materializes, (b) Primary API returns materialized, streaming is secondary, (c) Two equal methods, caller chooses - Recommendation: Option (c) - provide both `ExecuteSearchAsync` (streaming) and `ExecuteSearchToModelAsync` (materialized) - Impact: Excel export needs materialized; real-time progress benefits from streaming --- ### Phase 7: implement-excel-export **Q7.1: Test Data Source** - Context: Integration tests need sample search results to generate Excel files - Options: (a) Create mock data factories in test project, (b) Use shared test data from Phase 3, (c) Load sample data from JSON fixtures - Recommendation: Option (a) - mock data factories reusable across test projects - Impact: Test project structure and data sharing **Q7.2: Debug File Output Location** - Context: Legacy code wrote debug Excel files to disk for troubleshooting. Design includes `DebugWriteToFile` option. - Options: (a) Keep debug file feature but default to disabled, (b) Remove debug file feature, (c) Replace with structured logging - Recommendation: Option (a) - keep for backward compatibility, disabled by default - Impact: Configuration schema and troubleshooting capabilities **Q7.3: Empty Results Handling** - Context: What should happen when SearchResults is empty (not null)? - Options: (a) Generate sheet with headers only, (b) Generate sheet with "No results found" message, (c) Skip results sheet entirely - Recommendation: Option (a) - generate sheet with headers only (matches legacy behavior) - Impact: Sheet generation logic and user expectations **Q7.4: Large Export Memory Threshold** - Context: Design notes large exports (>100K rows) may need streaming approach - Options: (a) Implement now with configurable threshold, (b) Defer to future phase if memory issues arise, (c) Add memory monitoring without streaming - Recommendation: Option (b) - defer to future phase; current in-memory approach handles typical workloads - Impact: Scope of Phase 7 and future optimization work --- ## Codex MCP Review Findings (Phase 6-7) ### Phase 6 Codex Findings **F6.1: Work order traversal design conflict** (HIGH) - Finding: Base spec fixes 20 iterations with seed-list API, but change spec makes it configurable (`MaxTraversalIterations`) with `TraverseDownstreamAsync` - Action: Choose authoritative model and update spec/tasks - Recommendation: Keep configurable approach (more flexible), update base spec to align **F6.2: SqlKata parameter naming** (HIGH) - Finding: Delta spec requires `@p_*` named parameters and singleton compiler, but design uses `new SqlServerCompiler()` which defaults to `@p0` style - Action: Add parameter naming requirements to tasks, use singleton compiler pattern **F6.3: MIS behavior requirements missing from tasks** (MEDIUM) - Finding: MIS rules (always include non-match results when ExtractMisData=true, min-only/max-only timespan handling) not explicit in tasks - Action: Add explicit task items for MIS behavior requirements **F6.4: Filter handler priority order** (LOW) - Finding: Tasks don't require setting/validating priority order per spec delta - Action: Add explicit priority ordering requirements to filter handler tasks ### Phase 7 Codex Findings **F7.1: Sheet protection inconsistency** (HIGH) - Finding: Tasks call for protecting data sheets, spec says Search Results is unprotected, missing allowlist + unlocked 1000x1000 extension area - Action: Reconcile protection behavior between spec/design/tasks - Recommendation: Follow spec (Search Results unprotected), add allowlist + unlock range for other sheets **F7.2: ShowHeader handling missing** (HIGH) - Finding: Attribute-driven tables omit `ShowHeader` handling for merged section header rows (needed for criteria filter tables) - Action: Add ShowHeader support to AttributeTableWriter **F7.3: Project dependency name** (MEDIUM) - Finding: Tasks reference `JdeScoping.Domain` but NEW/src only has `JdeScoping.Core` - Action: Update dependency target to correct project name **F7.4: Multi-sheet rules incomplete** (MEDIUM) - Finding: No explicit requirements for sheet order, totals row disabled, empty-but-not-null MIS results - Action: Add explicit tasks/tests for multi-sheet behavior --- ### Phase 8: implement-web-api **Q8.1: LDAP Connection Pooling** - Context: Current design creates and disposes LdapConnection for each authentication attempt - Options: (a) Keep simple connection-per-request, (b) Implement connection pooling - Recommendation: Option (a) - connection-per-request for simplicity, authentication is infrequent - Impact: Performance for high-volume login scenarios **Q8.2: SignalR Authentication Requirement** - Context: Legacy StatusHub does not require authentication for connections - Options: (a) Keep hub open to all connections, (b) Require authentication for hub connections - Recommendation: Option (a) - keep open, status updates are not user-specific and user base is internal - Impact: Security posture for SignalR connections **Q8.3: Admin Bypass Configuration** - Context: Legacy code has hardcoded username bypass for group check ("dohertj2") - Options: (a) Remove entirely, (b) Make configurable via AuthOptions.AdminBypassUsers array - Recommendation: Option (b) - make configurable for dev flexibility without hardcoding - Impact: Developer experience during testing **Q8.4: File Upload Size Limits** - Context: Legacy has no explicit file size limits for Excel uploads - Options: (a) No limits, (b) Configure reasonable limit (e.g., 10MB) - Recommendation: Option (b) - configure 10MB limit to prevent abuse - Impact: User experience for bulk data uploads **Q8.5: CORS Configuration for Blazor WASM** - Context: If Blazor client is served from a different origin than API - Options: (a) Same-origin only, (b) Configure CORS for Blazor origin - Recommendation: Depends on deployment - same-origin if single host, CORS with explicit origins if separate - Impact: Blazor WASM deployment architecture --- ### Phase 9: implement-blazor-ui **Q9.1: Mock Services Strategy** - Context: Phase 8 (API) is not yet implemented. UI needs data to function. - Options: (a) Create mock services that return static/test data, (b) Create mock API endpoints in JdeScoping.Host, (c) Wait for Phase 8 before implementing UI - Recommendation: Option (a) - mock services allow parallel development - Impact: API client services can use mock implementations initially **Q9.2: Test Strategy for Blazor Components** - Context: bUnit is standard for Blazor component testing but adds complexity - Options: (a) Add bUnit tests as part of this phase, (b) Defer component testing to separate phase, (c) Manual testing only - Recommendation: Option (b) - focus on functional implementation first - Impact: No test tasks included in current tasks.md **Q9.3: Navigation Menu Visibility** - Context: Base spec shows minimal header with just title and user menu - Options: (a) Add navigation links to header, (b) Keep minimal header as specified, (c) Add collapsible sidebar navigation - Recommendation: Option (a) - header navigation for quick access - Impact: MainLayout.razor design --- ## Codex MCP Review Findings (Phase 8-9) ### Phase 8 Codex Findings **F8.1: NEW/src diverges from spec/plan** (HIGH) - Finding: Current auth API shape and LDAP behavior don't match spec (no `GetUserInfoAsync`, no multi-server/group checks), `/hubs/status` not mapped - Action: Align implementation plan with actual repo layout, add hub mapping **F8.2: SPA behaviors not in tasks** (HIGH) - Finding: CORS with credentials, JSON error responses, SignalR reconnection support are required but not planned - Action: Add explicit tasks for CORS, API error JSON behavior, SignalR reconnection **F8.3: LDAP scenarios incomplete** (MEDIUM) - Finding: Legacy has hardcoded group-bypass user, design adds `AdminBypassUsers` but doesn't use it, `GetUserInfoAsync` throws NotSupported - Action: Decide on admin bypass path, clarify GetUserInfoAsync behavior **F8.4: Project structure issues** (MEDIUM) - Finding: Tasks reference `JdeScoping.Domain` but only `JdeScoping.Core` exists; SearchController scheduled before StatusHub - Action: Align to actual project structure (use JdeScoping.Core), reorder dependencies **F8.5: SearchUpdate missing fields** (LOW) - Finding: Design omits `SubmitDT/StartDT/EndDT` fields that exist in spec/legacy model - Action: Add missing timestamp fields to SearchUpdate DTO ### Phase 9 Codex Findings **F9.1: Route conflict** (HIGH) - Finding: `Home.razor` owns `/`, but tasks add `/` to `Searches.razor` without removal/rename - Action: Remove template pages (Home.razor, Counter.razor, Weather.razor) before adding Searches **F9.2: JWT auth requirements incomplete** (HIGH) - Finding: Token attachment and 401 re-auth redirect required but not captured in auth tasks - Action: Add tasks for auth header injection and 401 redirect handling **F9.3: SearchStatus enum mismatch** (HIGH) - Finding: Spec uses New/Submitted/Started/Ended/Error but JdeScoping.Core uses Queued/Processing/Completed/Failed - Action: Decide on enum alignment - either map or standardize **F9.4: SearchEdit behaviors missing** (HIGH) - Finding: Search-type detection, submit confirmation, download results, Extract MIS checkbox required but not in tasks - Action: Expand SearchEdit tasks with spec behaviors **F9.5: SignalR reconnection requirements partial** (MEDIUM) - Finding: Backoff schedule + UI connection state + reconnect logging required but tasks only say "auto-reconnect" - Action: Add explicit tasks for reconnection UI and logging **F9.6: Clear Data confirmation missing** (MEDIUM) - Finding: Clear Data confirmations required per spec but filter panel tasks say "Clear empties grid" with no confirmation - Action: Add confirmation dialog to Clear Data functionality **F9.7: Template pages cleanup** (LOW) - Finding: Blazor template pages (Home, Counter, Weather, NavMenu) should be removed - Action: Add task to remove template pages as part of UI change set **F9.8: NuGet version mismatch** (LOW) - Finding: Base spec calls for Radzen 5.*/SignalR 9.* but design uses Radzen 8.4.2/SignalR 10.0.1 - Action: Update spec to reflect actual .NET 10 versions