Joseph Doherty
26ff8d9b4f
Set up repository with legacy .NET Framework 4.8 source (OLD/), new .NET 10 Blazor solution (NEW/), OpenSpec specifications, documentation, and project configuration.
296 lines
15 KiB
Markdown
296 lines
15 KiB
Markdown
# 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<T> pattern | Type-safe, hot-reload support |
|
|
| Logging | ILogger<T> 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<SearchResult>` streaming and `Task<SearchModel>` 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
|
|
|