diff --git a/.planning/phases/16-report-consolidation-toggle/16-RESEARCH.md b/.planning/phases/16-report-consolidation-toggle/16-RESEARCH.md new file mode 100644 index 0000000..7a08218 --- /dev/null +++ b/.planning/phases/16-report-consolidation-toggle/16-RESEARCH.md @@ -0,0 +1,380 @@ +# Phase 16: Report Consolidation Toggle - Research + +**Researched:** 2026-04-09 +**Domain:** WPF MVVM toggle / HTML export rendering / CSV export extension +**Confidence:** HIGH + +## Summary + +Phase 16 wires the `PermissionConsolidator.Consolidate()` helper from Phase 15 into the actual export flow via a user-visible checkbox. All foundational models (`ConsolidatedPermissionEntry`, `LocationInfo`, `UserAccessEntry`) and the consolidation algorithm are complete and tested. This phase is purely integration work: add a `MergePermissions` bool property to the ViewModel, bind a new "Export Options" GroupBox in both XAML views, and branch inside `BuildHtml` / `BuildCsv` / `WriteSingleFileAsync` based on that flag. + +The consolidation path for HTML requires rendering a different by-user table structure — one row per `ConsolidatedPermissionEntry` with a "Sites" column that shows either an inline site title (1 location) or an `[N sites]` badge that expands an inline sub-list via the existing `toggleGroup()` JS. The by-site view must be hidden when consolidation is ON. The CSV path replaces flat per-user rows with a "Locations" column that lists all site titles separated by semicolons (or similar). The by-site view is disabled by hiding/graying the view-toggle buttons in the HTML; the ViewModel `IsGroupByUser` WPF DataGrid grouping is unrelated (it controls the WPF DataGrid, not the HTML export). + +The session-global toggle lives as a single `[ObservableProperty] bool _mergePermissions` on `UserAccessAuditViewModel`. The `PermissionsViewModel` receives the same bool property as a no-op placeholder. Because both ViewModels are independent instances (no shared service is required), the CONTEXT.md decision of "session-scoped global setting" is implemented by putting the property in the ViewModel and never persisting it — it resets to `false` on app restart by default initialization. + +**Primary recommendation:** Add `MergePermissions` to `UserAccessAuditViewModel`, bind it in both XAML tabs, pass it to `BuildHtml`/`BuildCsv`, and add a consolidated rendering branch inside those services. Keep the non-consolidated path byte-identical to the current output (no structural changes to the existing code path). + +--- + + +## User Constraints (from CONTEXT.md) + +### Locked Decisions +| Decision | Value | +|---|---| +| Consolidation scope | User access audit report only — site-centric permission report is unchanged | +| Consolidation key | `UserLogin + PermissionLevel + AccessType + GrantedThrough` (4-field match) | +| Source model | `UserAccessEntry` (already normalized, one user per row) | +| Consolidation defaults OFF | Toggle must default to unchecked | +| No API calls | Pure data transformation via `PermissionConsolidator.Consolidate()` | +| Existing exports unchanged when OFF | Output must be identical to pre-v2.3 when toggle is OFF | + +### Discussed (Locked) +- **Toggle Placement**: New "Export Options" GroupBox in the left panel of both audit tabs, always visible; single `CheckBox` labeled "Merge duplicate permissions"; follows the same pattern as the existing "Scan Options" GroupBox (lines 199-210 of `UserAccessAuditView.xaml`) +- **Toggle applies to both HTML and CSV exports** (user override of REQUIREMENTS.md CSV exclusion) +- **Consolidated HTML rendering**: by-user view consolidated rows with "Sites" column; 1 location = inline title; 2+ locations = `[N sites]` badge expanding inline sub-list via existing `toggleGroup()` JS pattern; by-site view is disabled/hidden when consolidation is ON +- **Session persistence**: session-scoped property on ViewModel — resets to OFF on app restart; global state shared across all tabs (both ViewModels read/write same logical property) +- **PermissionsViewModel**: toggle present in UI but no-op — stores value, does not apply consolidation + +### Claude's Discretion +- Exact CSS for the `[N sites]` badge (follow existing `.badge` class style already in `UserAccessHtmlExportService.cs`) +- CSV consolidated column format (semicolon-separated site titles is the most natural approach) +- Whether `BuildHtml` takes a `bool mergePermissions` parameter or a richer options object (bool parameter is simpler given only one flag at this phase) + +### Deferred Ideas (OUT OF SCOPE) +- Site-centric consolidation logic (toggle present in UI but no-op for site-centric exports) +- Group expansion within consolidated rows (Phase 17) +- Persistent consolidation preference across app restarts (session-only for now) +- "Consolidated view" report header indicator (decided: not needed) + + +--- + + +## Phase Requirements + +| ID | Description | Research Support | +|----|-------------|-----------------| +| RPT-03 | User can enable/disable entry consolidation per export (toggle in export settings) | Toggle as `[ObservableProperty] bool _mergePermissions` in `UserAccessAuditViewModel`; `PermissionsViewModel` has same property as placeholder; both XAML views get "Export Options" GroupBox; `BuildHtml` and `BuildCsv`/`WriteSingleFileAsync` accept `bool mergePermissions` and branch accordingly | + + +--- + +## Standard Stack + +### Core +| Library | Version | Purpose | Why Standard | +|---------|---------|---------|--------------| +| CommunityToolkit.Mvvm | already referenced | `[ObservableProperty]` source gen for `MergePermissions` bool | Same pattern used for `IncludeInherited`, `ScanFolders`, `IncludeSubsites` throughout both ViewModels | +| WPF / XAML | .NET 10 Windows | GroupBox + CheckBox binding | Established UI pattern in this codebase | +| `PermissionConsolidator.Consolidate()` | Phase 15 (complete) | Transforms `IReadOnlyList` → `IReadOnlyList` | Purpose-built for this exact use case | + +### No New Dependencies +Phase 16 adds zero new NuGet packages. All required types and services are already present. + +## Architecture Patterns + +### Recommended Project Structure Changes + +``` +SharepointToolbox/ +├── ViewModels/Tabs/ +│ ├── UserAccessAuditViewModel.cs ← add MergePermissions [ObservableProperty] +│ └── PermissionsViewModel.cs ← add MergePermissions [ObservableProperty] (no-op) +├── Views/Tabs/ +│ ├── UserAccessAuditView.xaml ← add Export Options GroupBox after Scan Options GroupBox +│ └── PermissionsView.xaml ← add Export Options GroupBox after Display Options GroupBox +└── Services/Export/ + ├── UserAccessHtmlExportService.cs ← BuildHtml(entries, mergePermissions, branding) + └── UserAccessCsvExportService.cs ← BuildCsv(…, mergePermissions) + WriteSingleFileAsync(…, mergePermissions) + +SharepointToolbox.Tests/ +├── Services/Export/ +│ └── UserAccessHtmlExportServiceTests.cs ← add consolidated path tests +│ └── UserAccessCsvExportServiceTests.cs ← add consolidated path tests +└── ViewModels/ + └── UserAccessAuditViewModelTests.cs ← add MergePermissions property test +``` + +### Pattern 1: ObservableProperty Bool on ViewModel (CommunityToolkit.Mvvm) + +**What:** Declare a source-generated bool property that defaults to `false`. +**When to use:** Every scan/export option toggle in this codebase uses this pattern. + +```csharp +// In UserAccessAuditViewModel.cs — matches existing IncludeInherited / ScanFolders pattern +[ObservableProperty] +private bool _mergePermissions; +``` + +The source generator emits `MergePermissions` property with `OnPropertyChanged`. No partial handler needed unless additional side effects are required (none needed here). + +### Pattern 2: GroupBox + CheckBox in XAML (following Scan Options pattern) + +**What:** A new GroupBox labeled "Export Options" with a single CheckBox, placed below Scan Options in the left panel DockPanel. +**Reference location:** `UserAccessAuditView.xaml` lines 199-210 — the existing "Scan Options" GroupBox. + +```xml + + + + + + +``` + +**Localization note:** The project uses `TranslationSource.Instance` for all user-visible strings. New keys are required: `audit.grp.export` and `chk.merge.permissions` in both `Strings.resx` and `Strings.fr.resx`. + +### Pattern 3: Export Service Signature Extension + +**What:** Add `bool mergePermissions = false` parameter to `BuildHtml`, `BuildCsv`, and `WriteSingleFileAsync`. Callers in the ViewModel pass `MergePermissions`. +**Why a plain bool:** Only one flag at this phase; using a richer options object adds indirection with no benefit yet. + +```csharp +// UserAccessHtmlExportService +public string BuildHtml( + IReadOnlyList entries, + bool mergePermissions = false, + ReportBranding? branding = null) +{ + if (mergePermissions) + { + var consolidated = PermissionConsolidator.Consolidate(entries); + return BuildConsolidatedHtml(consolidated, entries, branding); + } + // existing path unchanged + ... +} +``` + +```csharp +// UserAccessCsvExportService +public async Task WriteSingleFileAsync( + IReadOnlyList entries, + string filePath, + CancellationToken ct, + bool mergePermissions = false) +{ + if (mergePermissions) + { + var consolidated = PermissionConsolidator.Consolidate(entries); + // write consolidated CSV rows + } + // existing path unchanged + ... +} +``` + +**ViewModel call sites** (in `ExportHtmlAsync` and `ExportCsvAsync`): +```csharp +await _htmlExportService.WriteAsync(Results, dialog.FileName, CancellationToken.None, branding, MergePermissions); +await _csvExportService.WriteSingleFileAsync(Results, dialog.FileName, CancellationToken.None, MergePermissions); +``` + +### Pattern 4: Consolidated HTML By-User View Structure + +**What:** When `mergePermissions = true`, the by-user table renders one row per `ConsolidatedPermissionEntry` with a new "Sites" column replacing the "Site" column. The by-site view toggle is hidden. + +**Sites column rendering logic:** +- `LocationCount == 1`: render `entry.Locations[0].SiteTitle` as plain text (no badge/expand) +- `LocationCount >= 2`: render `N sites` followed by sub-rows `data-group="loc{idx}"` each containing a link to the location's SiteUrl with SiteTitle label + +**Reuse of `toggleGroup()` JS:** The existing `toggleGroup(id)` function (line 276 in `UserAccessHtmlExportService.cs`) hides/shows rows by `data-group` attribute. Expandable location sub-lists use the same mechanism — each sub-row gets `data-group="loc{idx}"` and starts hidden. + +**By-site view suppression:** When consolidation is ON, omit the "By Site" button and `view-site` div from the HTML entirely (simplest approach — no dead HTML). Alternatively, render the button as disabled. Omitting is cleaner. + +**Column layout for consolidated by-user view:** +| Column | Source Field | +|--------|-------------| +| User | `UserDisplayName` (+ Guest badge if `IsExternalUser`) | +| Permission Level | `PermissionLevel` (+ high-priv icon if `IsHighPrivilege`) | +| Access Type | badge from `AccessType` | +| Granted Through | `GrantedThrough` | +| Sites | inline title OR `[N sites]` badge + expandable sub-list | + +### Pattern 5: Consolidated CSV Format + +**What:** When consolidation is ON, each `ConsolidatedPermissionEntry` becomes one row with a "Locations" column containing all site titles joined by `"; "`. + +**Column layout for consolidated CSV:** +``` +"User","User Login","Permission Level","Access Type","Granted Through","Locations","Location Count" +``` + +The "Locations" value: `string.Join("; ", entry.Locations.Select(l => l.SiteTitle))`. + +This is a distinct schema from the flat CSV, so the consolidated path writes its own header line. + +### Anti-Patterns to Avoid + +- **Modifying the existing non-consolidated code path in any way** — the success criterion is byte-for-byte identical output when toggle is OFF. Touch nothing in the existing rendering branches. +- **Using a shared service for MergePermissions state** — CONTEXT.md says session-scoped property on the ViewModel, not a singleton service. Both ViewModels have independent instances; no cross-ViewModel coordination is needed because site-centric is a no-op. +- **Re-using `IsGroupByUser` to control by-site view in HTML** — `IsGroupByUser` controls the WPF DataGrid grouping only; the HTML export is stateless and renders both views based solely on the `mergePermissions` flag. +- **Adding a partial handler for `OnMergePermissionsChanged`** — no side effects are needed on toggle change (no view refresh required, HTML export is computed at export time). + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Permission grouping / merging | Custom LINQ grouping | `PermissionConsolidator.Consolidate()` | Already unit-tested with 9 cases; handles case-insensitivity, ordering, LocationInfo construction | +| Toggle state persistence | Session service, file-backed setting | `[ObservableProperty] bool _mergePermissions` defaults to false | Session-scoped = default bool initialization; no persistence infrastructure needed | +| Expandable sub-list JS | New JS function | Existing `toggleGroup()` in `UserAccessHtmlExportService.cs` | Already handles show/hide of `data-group` rows; locations sub-list uses same mechanism with `loc{idx}` IDs | + +## Common Pitfalls + +### Pitfall 1: Breaking the Byte-Identical Guarantee +**What goes wrong:** Any change to the non-consolidated HTML rendering path (even whitespace changes to `sb.AppendLine` calls) breaks the "byte-for-byte identical" success criterion. +**Why it happens:** Developers add a parameter and reorganize the method, inadvertently altering the existing branches. +**How to avoid:** Add the consolidated branch as a separate early-return path (`if (mergePermissions) { ... return; }`). Leave the existing `StringBuilder` building code completely untouched below that branch. +**Warning signs:** A test that captures the current output (`BuildHtml(entries, mergePermissions: false)`) and compares it character-by-character to the pre-toggle output fails. + +### Pitfall 2: ID Collisions Between User Groups and Location Sub-lists +**What goes wrong:** The consolidated by-user view uses `ugrp{n}` IDs for user group headers and `loc{n}` IDs for expandable location sub-lists. If the counter resets between the two, `toggleGroup` clicks the wrong rows. +**Why it happens:** Two independent counters that both start at 0 use different prefixes — this is fine — but if a single counter is used for both, collisions occur. +**How to avoid:** Use a separate counter for location groups (`int locIdx = 0`) distinct from the user group counter. + +### Pitfall 3: WriteAsync Signature Mismatch +**What goes wrong:** `UserAccessHtmlExportService.WriteAsync` calls `BuildHtml` internally. If `BuildHtml` gets the new `mergePermissions` parameter but `WriteAsync` doesn't pass it through, the HTML file ignores the toggle. +**Why it happens:** `WriteAsync` is the method called by the ViewModel; `BuildHtml` is the method being extended. +**How to avoid:** Extend `WriteAsync` with the same `bool mergePermissions = false` parameter and pass it to `BuildHtml`. + +### Pitfall 4: CSV `BuildCsv` vs `WriteSingleFileAsync` +**What goes wrong:** The ViewModel calls `WriteSingleFileAsync` (single-file export). `BuildCsv` is per-user. Consolidation applies at the whole-collection level, so only `WriteSingleFileAsync` needs a consolidated path. Adding a consolidated path to `BuildCsv` (per-user) is incorrect — a consolidated row already spans users. +**Why it happens:** Developer sees `BuildCsv` and `WriteSingleFileAsync` and tries to add consolidation to both. +**How to avoid:** Add the consolidated branch only to `WriteSingleFileAsync`. `BuildCsv` is not called by the consolidation path. The per-user export (`WriteAsync`) is also not called by the ViewModel's export command (`ExportCsvAsync` calls `WriteSingleFileAsync` only). + +### Pitfall 5: Localization Keys Missing from French .resx +**What goes wrong:** New string keys added to `Strings.resx` but not to `Strings.fr.resx` cause `TranslationSource.Instance[key]` to return an empty string in French locale. +**Why it happens:** Developers add only the default `.resx`. +**How to avoid:** Add both `audit.grp.export` and `chk.merge.permissions` to both `.resx` files in the same task. + +## Code Examples + +### Current Export Call Sites (ViewModel — to be extended) + +```csharp +// ExportHtmlAsync — current (line 526 of UserAccessAuditViewModel.cs) +await _htmlExportService.WriteAsync(Results, dialog.FileName, CancellationToken.None, branding); + +// ExportCsvAsync — current (line 494) +await _csvExportService.WriteSingleFileAsync(Results, dialog.FileName, CancellationToken.None); +``` + +After Phase 16, becomes: +```csharp +await _htmlExportService.WriteAsync(Results, dialog.FileName, CancellationToken.None, branding, MergePermissions); +await _csvExportService.WriteSingleFileAsync(Results, dialog.FileName, CancellationToken.None, MergePermissions); +``` + +### Existing toggleGroup JS (reuse as-is) + +```javascript +// Source: UserAccessHtmlExportService.cs inline JS, line 276-279 +function toggleGroup(id) { + var rows = document.querySelectorAll('tr[data-group="' + id + '"]'); + var isHidden = rows.length > 0 && rows[0].style.display === 'none'; + rows.forEach(function(r) { r.style.display = isHidden ? '' : 'none'; }); +} +``` + +Note: the current source has `""` doubled quotes in the selector (`'tr[data-group=""' + id + '""]'`) — this is a C# string literal escaping artifact. When rendered to HTML the output is correct single-quoted attribute selectors. Location sub-list rows must use the same `data-group` attribute format. + +### Consolidated HTML Sites Column — location sub-rows pattern + +```html + +HR Site + + +3 sites + + + + HR Site + + + + + Finance Site + + +``` + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| `BuildHtml(entries, branding)` | `BuildHtml(entries, mergePermissions, branding)` | Phase 16 | Callers must pass the flag | +| `WriteSingleFileAsync(entries, path, ct)` | `WriteSingleFileAsync(entries, path, ct, mergePermissions)` | Phase 16 | All callers (just the ViewModel) must pass the flag | +| No export option panel in XAML | Export Options GroupBox visible in both tabs | Phase 16 | Both XAML views change; both ViewModels gain the property | + +## Open Questions + +1. **`WriteAsync` vs `BuildHtml` parameter ordering** + - What we know: `WriteAsync` current signature is `(entries, filePath, ct, branding?)` — branding is already optional + - What's unclear: Should `mergePermissions` go before or after `branding` in the parameter list? + - Recommendation: Place `mergePermissions = false` after `ct` and before `branding?` so it's grouped with behavioral flags, not rendering decorations: `WriteAsync(entries, filePath, ct, mergePermissions = false, branding = null)` + +2. **French translation for "Merge duplicate permissions"** + - What we know: French `.resx` exists and all existing keys have French equivalents + - What's unclear: Exact French translation (developer decision) + - Recommendation: Use "Fusionner les permissions en double" — consistent with existing terminology in the codebase + +## Validation Architecture + +### Test Framework +| Property | Value | +|----------|-------| +| Framework | xUnit (project: `SharepointToolbox.Tests`) | +| Config file | `SharepointToolbox.Tests/SharepointToolbox.Tests.csproj` | +| Quick run command | `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests" --no-build` | +| Full suite command | `dotnet test` | + +### Phase Requirements → Test Map + +| Req ID | Behavior | Test Type | Automated Command | File Exists? | +|--------|----------|-----------|-------------------|-------------| +| RPT-03-a | `MergePermissions` defaults to `false` on ViewModel | unit | `dotnet test --filter "FullyQualifiedName~UserAccessAuditViewModelTests"` | ✅ (extend existing) | +| RPT-03-b | `BuildHtml(entries, false)` output is byte-identical to current output | unit | `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests"` | ✅ (extend existing) | +| RPT-03-c | `BuildHtml(entries, true)` includes consolidated rows and "Sites" column | unit | `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests"` | ✅ (extend existing) | +| RPT-03-d | `BuildHtml(entries, true)` with 2+ locations renders `[N sites]` badge | unit | `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests"` | ✅ (extend existing) | +| RPT-03-e | `BuildHtml(entries, true)` omits by-site view toggle | unit | `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests"` | ✅ (extend existing) | +| RPT-03-f | `WriteSingleFileAsync(entries, path, ct, false)` output unchanged | unit | `dotnet test --filter "FullyQualifiedName~UserAccessCsvExportServiceTests"` | ✅ (extend existing) | +| RPT-03-g | `WriteSingleFileAsync(entries, path, ct, true)` writes consolidated CSV rows | unit | `dotnet test --filter "FullyQualifiedName~UserAccessCsvExportServiceTests"` | ✅ (extend existing) | + +### Sampling Rate +- **Per task commit:** `dotnet test --filter "FullyQualifiedName~UserAccessHtmlExportServiceTests|FullyQualifiedName~UserAccessCsvExportServiceTests|FullyQualifiedName~UserAccessAuditViewModelTests" --no-build` +- **Per wave merge:** `dotnet test` +- **Phase gate:** Full suite green before `/gsd:verify-work` + +### Wave 0 Gaps +None — existing test infrastructure covers all phase requirements. No new test files needed; existing test files are extended with new test methods. + +## Sources + +### Primary (HIGH confidence) +- Direct inspection of `UserAccessHtmlExportService.cs` — full rendering pipeline, `toggleGroup()` JS, `BuildHtml`/`WriteAsync` signatures +- Direct inspection of `UserAccessCsvExportService.cs` — `BuildCsv`, `WriteSingleFileAsync`, `WriteAsync` signatures +- Direct inspection of `UserAccessAuditViewModel.cs` — `[ObservableProperty]` pattern, `ExportCsvAsync`, `ExportHtmlAsync` call sites +- Direct inspection of `PermissionsViewModel.cs` — confirms independent ViewModel, no shared state service +- Direct inspection of `UserAccessAuditView.xaml` — Scan Options GroupBox pattern at lines 199-210 +- Direct inspection of `PermissionsView.xaml` — Display Options GroupBox pattern and left panel structure +- Direct inspection of `PermissionConsolidator.cs` + `ConsolidatedPermissionEntry.cs` + `LocationInfo.cs` — complete Phase 15 API +- Direct inspection of `PermissionConsolidatorTests.cs` — 9 test cases confirming consolidator behavior +- Direct inspection of `Strings.resx` — existing localization key naming convention + +### Secondary (MEDIUM confidence) +- CommunityToolkit.Mvvm `[ObservableProperty]` source generation behavior — confirmed by existing usage of `_includeInherited`, `_scanFolders`, `_mergePermissions` pattern throughout codebase + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH — all dependencies already in the project; no new libraries +- Architecture: HIGH — all patterns directly observed in existing code +- Pitfalls: HIGH — derived from direct reading of the code paths being modified +- Test map: HIGH — existing test file locations and xUnit patterns confirmed + +**Research date:** 2026-04-09 +**Valid until:** 2026-05-09 (stable codebase, no fast-moving dependencies)