From fcb42b9bf99a6342ff59b6249a4dcf42f1c5ed00 Mon Sep 17 00:00:00 2001 From: Timothy Spaulding Date: Fri, 24 Jul 2026 21:43:57 -0400 Subject: [PATCH 1/6] Server rules: read-only view in the Rules Manager + subset/accessibility fixes (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First user-facing slice of server-rule support: a read-only view of a Graph account's Exchange messageRules inside the Rules Manager, so the rules can be seen and the mapping validated against real data before editing is built. Also folds in fixes found while validating against a live 68-rule mailbox. Wiring: - App.xaml.cs constructs GraphServerRuleService (reuses the shared GraphClient, no new disposables) and passes it to MainWindow. - MainWindow.OpenRulesManager builds a ServerRulesViewModel for the Graph account (with the cached folder list) and hands it to the window. - RulesManagerWindow shows a collapsible read-only "Server rules" section: a list (each row announces name, state, "runs on the server", any markers, and a one-line summary) and a read-only detail pane. Hidden when no Graph account. Subset expansion (every rule flagged on the test mailbox is now representable): - Added conditions bodyContains and sentToAddresses (mirroring subjectContains / fromAddresses, including multi-value gating for bodyContains). These two covered all 13 previously-flagged rules on the live mailbox. - Added the copyToFolder action. - Move/copy targets now resolve to the folder's display name (via the account's cached folders; Graph stores the folder id as FullName) instead of "another folder". View fidelity / accessibility: - DetailText reformatted: one condition/action per line under "Applies when:" / "Does:" headers, with a "Reason for block:" section only when a rule is read-only, errored, or not fully editable — and it now SHOWS the values of conditions even when they can't be edited (previously a hidden condition like "body contains 'X'" was omitted, which read as a mismatch). - Not-editable rules announce it per row, and the status line reports the count. - Detail pane is a read-only multi-line caret-navigable box (AcceptsReturn + IsReadOnlyCaretVisible) so a screen reader can arrow through every line. - Added F6 / Shift+F6 pane cycling (list, server list, server detail, status), and made the status lines focusable so the counts can be read on demand — the window previously had no F6 ring (New Window Checklist gap). - Empty-state focus lands on whichever list actually has rules. Diagnostic: /debug logs the unsupported field NAMES per flagged rule (names only, never raw predicate values, to keep mail subjects/addresses out of the log). Tests: +6 (bodyContains/sentToAddresses editable + round-trip, copyToFolder round-trip, folder-name resolution, the not-editable row marker, unsupported predicate now uses headerContains). Full suite 1587 passed; the lone red is the known clipboard flake (passes in isolation). Co-Authored-By: Claude Opus 4.8 --- .../GraphServerRuleServiceTests.cs | 66 ++++++++++++- QuickMail.Tests/ServerRulesViewModelTests.cs | 30 ++++++ QuickMail/App.xaml.cs | 5 +- QuickMail/Models/ServerRuleModel.cs | 75 ++++++++++---- QuickMail/Services/Graph/ServerRuleMapper.cs | 17 ++-- QuickMail/Services/GraphServerRuleService.cs | 5 + QuickMail/ViewModels/ServerRulesViewModel.cs | 37 ++++++- QuickMail/Views/MainWindow.xaml.cs | 13 ++- QuickMail/Views/RulesManagerWindow.xaml | 57 ++++++++++- QuickMail/Views/RulesManagerWindow.xaml.cs | 98 ++++++++++++++++++- 10 files changed, 361 insertions(+), 42 deletions(-) diff --git a/QuickMail.Tests/GraphServerRuleServiceTests.cs b/QuickMail.Tests/GraphServerRuleServiceTests.cs index 8501f6e2..62f2d892 100644 --- a/QuickMail.Tests/GraphServerRuleServiceTests.cs +++ b/QuickMail.Tests/GraphServerRuleServiceTests.cs @@ -134,14 +134,65 @@ public async Task List_UnsupportedPredicate_MarksRuleViewOnly() var handler = new RecordingHandler(Json(Collection( """ { "id": "r1", "displayName": "Complex", "sequence": 1, "isEnabled": true, - "conditions": { "subjectContains": ["x"], "bodyContains": ["secret"] }, + "conditions": { "subjectContains": ["x"], "headerContains": ["X-Spam"] }, "actions": { "markAsRead": true } } """))); var rule = (await Service(handler).ListAsync(_accountId)).Single(); Assert.False(rule.IsFullyEditable); - Assert.Contains("body contains", rule.UnsupportedFields); + Assert.Contains("header contains", rule.UnsupportedFields); + } + + [Fact] + public async Task List_BodyContainsAndSentToAddresses_AreEditable_AndRoundTrip() + { + // #333: these two conditions were added to the supported subset (they cover every rule that + // was previously flagged on a real O365 mailbox). Read them, and prove Create writes them + // back so an edit can't drop them. + var handler = new RecordingHandler( + Json(Collection( + """ + { "id": "r1", "displayName": "Boards", "sequence": 1, "isEnabled": true, + "conditions": { "subjectContains": ["Agenda"], "bodyContains": ["PTAC"], + "sentToAddresses": [ { "emailAddress": { "address": "board@contoso.com" } } ] }, + "actions": { "moveToFolder": "F", "stopProcessingRules": true } } + """)), + Json("""{ "id": "new", "displayName": "Boards" }""")); + var svc = Service(handler); + + var rule = (await svc.ListAsync(_accountId)).Single(); + Assert.True(rule.IsFullyEditable); // no longer flagged + Assert.Empty(rule.UnsupportedFields); + Assert.Equal("PTAC", rule.BodyContains); + Assert.Equal("board@contoso.com", Assert.Single(rule.SentToAddresses)); + + await svc.CreateAsync(_accountId, rule); + var body = handler.Bodies.Last()!; + Assert.Contains("bodyContains", body); + Assert.Contains("sentToAddresses", body); + Assert.Contains("board@contoso.com", body); + } + + [Fact] + public async Task List_CopyToFolder_IsEditable_AndRoundTrips() + { + var handler = new RecordingHandler( + Json(Collection( + """ + { "id": "r1", "displayName": "Archive copy", "sequence": 1, "isEnabled": true, + "conditions": { "subjectContains": ["x"] }, + "actions": { "copyToFolder": "FolderZ" } } + """)), + Json("""{ "id": "new" }""")); + var svc = Service(handler); + + var rule = (await svc.ListAsync(_accountId)).Single(); + Assert.True(rule.IsFullyEditable); + Assert.Equal("FolderZ", rule.CopyToFolderId); + + await svc.CreateAsync(_accountId, rule); + Assert.Contains("copyToFolder", handler.Bodies.Last()!); } [Fact] @@ -163,6 +214,7 @@ public async Task List_UnsupportedAction_MarksRuleViewOnly() [Theory] [InlineData("subjectContains")] [InlineData("senderContains")] + [InlineData("bodyContains")] [InlineData("bodyOrSubjectContains")] public async Task List_MultiValueStringPredicate_IsViewOnly(string predicate) { @@ -444,6 +496,16 @@ public void ToString_CarriesNameStateAndSummary() Assert.DoesNotContain("ServerRuleModel", text); // never the type name } + [Fact] + public void ToString_MarksNotEditableRules_SoTheListRowAnnouncesIt() + { + var editable = new ServerRuleModel { DisplayName = "Simple", SubjectContains = "x", MarkAsRead = true }; + var notEditable = new ServerRuleModel { DisplayName = "Complex", IsFullyEditable = false, UnsupportedFields = ["body contains"] }; + + Assert.DoesNotContain("not editable", editable.ToString(), StringComparison.OrdinalIgnoreCase); + Assert.Contains("not editable in QuickMail", notEditable.ToString(), StringComparison.OrdinalIgnoreCase); + } + [Fact] public void DetailText_ExplainsWhyARuleIsNotEditable() { diff --git a/QuickMail.Tests/ServerRulesViewModelTests.cs b/QuickMail.Tests/ServerRulesViewModelTests.cs index 9456d3ce..5066612e 100644 --- a/QuickMail.Tests/ServerRulesViewModelTests.cs +++ b/QuickMail.Tests/ServerRulesViewModelTests.cs @@ -316,6 +316,36 @@ public async Task Move_WhenServerRefuses_RollsBackLocalOrder() Assert.Equal(["a", "b"], vm.Rules.Select(r => r.Id)); // order restored } + [Fact] + public async Task Refresh_ResolvesMoveAndCopyFolderNames_FromCachedFolders() + { + var rule = new ServerRuleModel + { + Id = "r1", DisplayName = "Filer", + MoveToFolderId = "graph-id-move", + CopyToFolderId = "graph-id-copy", + SubjectContains = "x", + }; + var svc = new FakeServerRuleService { Stored = [rule] }; + var folders = new Dictionary> + { + [_accountId] = + [ + new MailFolderModel { FullName = "graph-id-move", DisplayName = "Archive" }, + new MailFolderModel { FullName = "graph-id-copy", DisplayName = "Backups" }, + ], + }; + var vm = new ServerRulesViewModel(svc, [GraphAccount()], folders); + + await vm.RefreshCommand.ExecuteAsync(null); + + var loaded = vm.Rules.Single(); + Assert.Equal("Archive", loaded.MoveToFolderName); + Assert.Equal("Backups", loaded.CopyToFolderName); + Assert.Contains("move to Archive", loaded.OneLineSummary()); + Assert.Contains("copy to Backups", loaded.OneLineSummary()); + } + [Fact] public void HasGraphAccount_FalseWithoutAGraphAccount() { diff --git a/QuickMail/App.xaml.cs b/QuickMail/App.xaml.cs index 31ff4821..0e8c9e62 100644 --- a/QuickMail/App.xaml.cs +++ b/QuickMail/App.xaml.cs @@ -230,6 +230,9 @@ IMailService BackendFor(AccountModel a) var templateService = _templateService; // accountService drives the one-time "All accounts" → per-account rule migration (#333 D1). var ruleService = new RuleService(mailRouter, localStore, profile.ProfileDir, accountService); + // Server-side (Exchange/Graph) Inbox rules — read/manage a Graph account's messageRules. + // Reuses the shared GraphClient (no own disposables), so no disposal wiring needed. + var serverRuleService = new GraphServerRuleService(accountService, graphBackend.Client); var syncService = new SyncService(mailRouter, localStore, configService, ruleService); // Contact sync (issue #256): Graph source reuses the Graph backend's client; Google source @@ -295,7 +298,7 @@ IMailService BackendFor(AccountModel a) mainVm.RegisterAccountBackend = a => mailRouter.RegisterAccount(a.Id, BackendFor(a)); mainVm.LoadAccountList(accounts); - var mainWindow = new MainWindow(mainVm, smtpService, accountService, credentialService, mailRouter, oauthService, commandRegistry, contactService, configService, localStore, viewService, ruleService, templateService, featureGate, flagService, customDictionary, themeService, _bugReportService, _notificationService, contactSyncService, graphCalendarSync); + var mainWindow = new MainWindow(mainVm, smtpService, accountService, credentialService, mailRouter, oauthService, commandRegistry, contactService, configService, localStore, viewService, ruleService, templateService, featureGate, flagService, customDictionary, themeService, _bugReportService, _notificationService, contactSyncService, graphCalendarSync, serverRuleService); // Clicking a new-mail toast brings QuickMail to the foreground and opens the referenced // message. OnActivated may fire on a background thread, so marshal to the UI thread first. diff --git a/QuickMail/Models/ServerRuleModel.cs b/QuickMail/Models/ServerRuleModel.cs index 4790a45e..2ee28522 100644 --- a/QuickMail/Models/ServerRuleModel.cs +++ b/QuickMail/Models/ServerRuleModel.cs @@ -38,8 +38,16 @@ public sealed class ServerRuleModel public string? SenderContains { get; set; } public List FromAddresses { get; set; } = []; + + /// Recipients the message was sent to (Graph sentToAddresses). + public List SentToAddresses { get; set; } = []; + public string? SubjectContains { get; set; } public string? BodyOrSubjectContains { get; set; } + + /// Text matched against the message body (Graph bodyContains). + public string? BodyContains { get; set; } + public bool SentToMe { get; set; } public bool SentOnlyToMe { get; set; } public bool HasAttachments { get; set; } @@ -55,6 +63,12 @@ public sealed class ServerRuleModel /// Display name for , resolved for prose. Not sent to Graph. public string? MoveToFolderName { get; set; } + /// Graph folder ID for a copy action (copyToFolder). + public string? CopyToFolderId { get; set; } + + /// Display name for , resolved for prose. Not sent to Graph. + public string? CopyToFolderName { get; set; } + public bool MarkAsRead { get; set; } /// "low", "normal", or "high". Null when the rule doesn't set importance. @@ -99,6 +113,9 @@ public override string ToString() var parts = new List { name, IsEnabled ? "enabled" : "disabled" }; if (IsReadOnly) parts.Add("read-only"); if (HasError) parts.Add("error"); + // Announced per row so a user arrowing the list hears which rules QuickMail can't fully edit + // (they use conditions/actions outside the editable subset). Read-only rules already say so. + if (!IsFullyEditable && !IsReadOnly) parts.Add("not editable in QuickMail"); var summary = OneLineSummary(); var head = string.Join(", ", parts); @@ -125,42 +142,58 @@ public string OneLineSummary() public string DetailText() { var sb = new StringBuilder(); - sb.Append(string.IsNullOrWhiteSpace(DisplayName) ? "Unnamed rule" : DisplayName); - sb.Append(IsEnabled ? " (enabled)" : " (disabled)"); - sb.AppendLine(); - - var conditions = DescribeConditions(); - sb.AppendLine(conditions.Count == 0 - ? "Applies to: all messages." - : "Applies when: " + string.Join("; ", conditions) + "."); - - var actions = DescribeActions(); - sb.AppendLine(actions.Count == 0 - ? "Does: nothing." - : "Does: " + string.Join("; ", actions) + "."); + var name = string.IsNullOrWhiteSpace(DisplayName) ? "Unnamed rule" : DisplayName; + sb.AppendLine($"{name} ({(IsEnabled ? "enabled" : "disabled")})"); - if (IsReadOnly) sb.AppendLine("This rule is read-only on the server and cannot be changed."); - if (HasError) sb.AppendLine("This rule is in an error state on the server."); + // Each condition / action on its own line under a header, so it's easy to read line by line + // with a screen reader. + AppendSection(sb, "Applies when:", DescribeConditions(), "all messages"); + AppendSection(sb, "Does:", DescribeActions(), "nothing"); + var reasons = new List(); + if (IsReadOnly) reasons.Add("This rule is read-only on the server and cannot be changed."); + if (HasError) reasons.Add("This rule is in an error state on the server."); if (!IsFullyEditable) + reasons.Add(UnsupportedFields.Count > 0 + ? $"This rule uses conditions or actions QuickMail can't edit yet ({string.Join(", ", UnsupportedFields)}). You can enable, disable, or delete it here, or edit it in Outlook." + : "This rule uses conditions or actions QuickMail can't edit yet. You can enable, disable, or delete it here, or edit it in Outlook."); + + if (reasons.Count > 0) { - sb.Append("This rule uses "); - sb.Append(UnsupportedFields.Count > 0 - ? "conditions or actions QuickMail can't edit yet (" + string.Join(", ", UnsupportedFields) + ")" - : "conditions or actions QuickMail can't edit yet"); - sb.AppendLine(". You can enable, disable, or delete it here, or edit it in Outlook."); + sb.AppendLine("Reason for block:"); + foreach (var r in reasons) sb.AppendLine(r); } return sb.ToString().TrimEnd(); } + /// + /// Appends a titled section with one item per line. Non-empty: the header on its own line, then + /// each item on its own line separated by ";" (no trailing ";" on the last). Empty: the header + /// and the empty text on one line ("Applies when: all messages"). + /// + private static void AppendSection(StringBuilder sb, string header, List items, string emptyText) + { + if (items.Count == 0) + { + sb.AppendLine($"{header} {emptyText}"); + return; + } + + sb.AppendLine(header); + for (var i = 0; i < items.Count; i++) + sb.AppendLine(i < items.Count - 1 ? $"{items[i]};" : items[i]); + } + private List DescribeConditions() { var c = new List(); if (!string.IsNullOrWhiteSpace(SenderContains)) c.Add($"sender contains '{SenderContains}'"); if (FromAddresses.Count > 0) c.Add($"from {string.Join(" or ", FromAddresses)}"); + if (SentToAddresses.Count > 0) c.Add($"sent to {string.Join(" or ", SentToAddresses)}"); if (!string.IsNullOrWhiteSpace(SubjectContains)) c.Add($"subject contains '{SubjectContains}'"); if (!string.IsNullOrWhiteSpace(BodyOrSubjectContains)) c.Add($"subject or body contains '{BodyOrSubjectContains}'"); + if (!string.IsNullOrWhiteSpace(BodyContains)) c.Add($"body contains '{BodyContains}'"); if (SentToMe) c.Add("sent to me"); if (SentOnlyToMe) c.Add("sent only to me"); if (HasAttachments) c.Add("has attachments"); @@ -173,6 +206,8 @@ private List DescribeActions() var a = new List(); if (!string.IsNullOrWhiteSpace(MoveToFolderId)) a.Add($"move to {(string.IsNullOrWhiteSpace(MoveToFolderName) ? "another folder" : MoveToFolderName)}"); + if (!string.IsNullOrWhiteSpace(CopyToFolderId)) + a.Add($"copy to {(string.IsNullOrWhiteSpace(CopyToFolderName) ? "another folder" : CopyToFolderName)}"); if (MarkAsRead) a.Add("mark as read"); if (!string.IsNullOrWhiteSpace(MarkImportance)) a.Add($"set importance to {MarkImportance}"); if (Delete) a.Add("move to Deleted Items"); diff --git a/QuickMail/Services/Graph/ServerRuleMapper.cs b/QuickMail/Services/Graph/ServerRuleMapper.cs index 046f6e5c..2266f7f0 100644 --- a/QuickMail/Services/Graph/ServerRuleMapper.cs +++ b/QuickMail/Services/Graph/ServerRuleMapper.cs @@ -24,14 +24,14 @@ internal static class ServerRuleMapper /// Condition predicates QuickMail can represent and therefore safely rewrite. internal static readonly HashSet SupportedConditions = new(StringComparer.OrdinalIgnoreCase) { - "senderContains", "fromAddresses", "subjectContains", "bodyOrSubjectContains", - "sentToMe", "sentOnlyToMe", "hasAttachments", "importance", + "senderContains", "fromAddresses", "sentToAddresses", "subjectContains", "bodyContains", + "bodyOrSubjectContains", "sentToMe", "sentOnlyToMe", "hasAttachments", "importance", }; /// Actions QuickMail can represent and therefore safely rewrite. internal static readonly HashSet SupportedActions = new(StringComparer.OrdinalIgnoreCase) { - "moveToFolder", "markAsRead", "markImportance", "delete", "forwardTo", "stopProcessingRules", + "moveToFolder", "copyToFolder", "markAsRead", "markImportance", "delete", "forwardTo", "stopProcessingRules", }; /// @@ -47,16 +47,14 @@ internal static class ServerRuleMapper /// private static readonly HashSet SingleValueStringPredicates = new(StringComparer.OrdinalIgnoreCase) { - "senderContains", "subjectContains", "bodyOrSubjectContains", + "senderContains", "subjectContains", "bodyContains", "bodyOrSubjectContains", }; /// Friendlier labels for the predicates/actions we don't support yet. private static readonly Dictionary FriendlyNames = new(StringComparer.OrdinalIgnoreCase) { - ["bodyContains"] = "body contains", ["headerContains"] = "header contains", ["recipientContains"] = "recipient contains", - ["sentToAddresses"] = "sent to specific addresses", ["sentCcMe"] = "sent CC to me", ["sentToOrCcMe"] = "sent to or CC me", ["notSentToMe"] = "not sent to me", @@ -75,7 +73,6 @@ internal static class ServerRuleMapper ["isApprovalRequest"] = "approval request", ["isAutomaticReply"] = "automatic reply", ["isNonDeliveryReport"] = "non-delivery report", - ["copyToFolder"] = "copy to folder", ["forwardAsAttachmentTo"] = "forward as attachment", ["redirectTo"] = "redirect to", ["assignCategories"] = "assign categories", @@ -122,8 +119,10 @@ internal static ServerRuleModel ToModel(GraphMessageRule dto) { case "sendercontains": m.SenderContains = FirstString(p.Value); break; case "subjectcontains": m.SubjectContains = FirstString(p.Value); break; + case "bodycontains": m.BodyContains = FirstString(p.Value); break; case "bodyorsubjectcontains": m.BodyOrSubjectContains = FirstString(p.Value); break; case "fromaddresses": m.FromAddresses = Recipients(p.Value); break; + case "senttoaddresses": m.SentToAddresses = Recipients(p.Value); break; case "senttome": m.SentToMe = p.Value.ValueKind == JsonValueKind.True; break; case "sentonlytome": m.SentOnlyToMe = p.Value.ValueKind == JsonValueKind.True; break; case "hasattachments": m.HasAttachments = p.Value.ValueKind == JsonValueKind.True; break; @@ -142,6 +141,7 @@ internal static ServerRuleModel ToModel(GraphMessageRule dto) switch (p.Name.ToLowerInvariant()) { case "movetofolder": m.MoveToFolderId = p.Value.GetString(); break; + case "copytofolder": m.CopyToFolderId = p.Value.GetString(); break; case "markasread": m.MarkAsRead = p.Value.ValueKind == JsonValueKind.True; break; case "markimportance": m.MarkImportance = p.Value.GetString(); break; case "delete": m.Delete = p.Value.ValueKind == JsonValueKind.True; break; @@ -177,8 +177,10 @@ internal static ServerRuleModel ToModel(GraphMessageRule dto) var conditions = new Dictionary(); if (!string.IsNullOrWhiteSpace(rule.SenderContains)) conditions["senderContains"] = new[] { rule.SenderContains }; if (!string.IsNullOrWhiteSpace(rule.SubjectContains)) conditions["subjectContains"] = new[] { rule.SubjectContains }; + if (!string.IsNullOrWhiteSpace(rule.BodyContains)) conditions["bodyContains"] = new[] { rule.BodyContains }; if (!string.IsNullOrWhiteSpace(rule.BodyOrSubjectContains)) conditions["bodyOrSubjectContains"] = new[] { rule.BodyOrSubjectContains }; if (rule.FromAddresses.Count > 0) conditions["fromAddresses"] = rule.FromAddresses.Select(ToRecipient).ToArray(); + if (rule.SentToAddresses.Count > 0) conditions["sentToAddresses"] = rule.SentToAddresses.Select(ToRecipient).ToArray(); if (rule.SentToMe) conditions["sentToMe"] = true; if (rule.SentOnlyToMe) conditions["sentOnlyToMe"] = true; if (rule.HasAttachments) conditions["hasAttachments"] = true; @@ -186,6 +188,7 @@ internal static ServerRuleModel ToModel(GraphMessageRule dto) var actions = new Dictionary(); if (!string.IsNullOrWhiteSpace(rule.MoveToFolderId)) actions["moveToFolder"] = rule.MoveToFolderId; + if (!string.IsNullOrWhiteSpace(rule.CopyToFolderId)) actions["copyToFolder"] = rule.CopyToFolderId; if (rule.MarkAsRead) actions["markAsRead"] = true; if (!string.IsNullOrWhiteSpace(rule.MarkImportance)) actions["markImportance"] = rule.MarkImportance; if (rule.Delete) actions["delete"] = true; diff --git a/QuickMail/Services/GraphServerRuleService.cs b/QuickMail/Services/GraphServerRuleService.cs index 4557152a..569d99af 100644 --- a/QuickMail/Services/GraphServerRuleService.cs +++ b/QuickMail/Services/GraphServerRuleService.cs @@ -50,6 +50,11 @@ public async Task> ListAsync(Guid accountId, Canc .ToList(); LogService.Debug($"ServerRules: listed {rules.Count} rule(s) for {account.Username} " + $"({rules.Count(r => !r.IsFullyEditable)} not fully editable)"); + // Diagnostic (/debug only): name the specific unsupported field(s) per flagged rule so a + // gap in the supported subset can be identified. Field NAMES only — not the raw predicate + // values, which would put mail subjects/addresses in the log. + foreach (var r in rules.Where(x => !x.IsFullyEditable)) + LogService.Debug($"ServerRules: not-editable '{r.DisplayName}' unsupported=[{string.Join(", ", r.UnsupportedFields)}]"); return rules; } diff --git a/QuickMail/ViewModels/ServerRulesViewModel.cs b/QuickMail/ViewModels/ServerRulesViewModel.cs index 65c5c61f..a0cffbf5 100644 --- a/QuickMail/ViewModels/ServerRulesViewModel.cs +++ b/QuickMail/ViewModels/ServerRulesViewModel.cs @@ -41,9 +41,15 @@ public partial class ServerRulesViewModel : ObservableObject // ── Construction ──────────────────────────────────────────────────────── - public ServerRulesViewModel(IServerRuleService service, IEnumerable graphAccounts) + private readonly IReadOnlyDictionary>? _foldersByAccount; + + public ServerRulesViewModel( + IServerRuleService service, + IEnumerable graphAccounts, + IReadOnlyDictionary>? foldersByAccount = null) { _service = service; + _foldersByAccount = foldersByAccount; AccountOptions = graphAccounts .Where(a => a.BackendKind == BackendKind.MicrosoftGraph) @@ -53,6 +59,23 @@ public ServerRulesViewModel(IServerRuleService service, IEnumerable + /// Fills in the display names for a rule's move/copy target folders. Graph rules carry only the + /// opaque folder ID; the cached folder list maps that ID (stored as FullName) to a + /// readable name. Best-effort — a target not in the cached set (e.g. a rarely-synced subfolder) + /// falls back to "another folder". + /// + private void ResolveFolderNames(ServerRuleModel rule) + { + if (SelectedAccount?.Id is not Guid accountId) return; + if (_foldersByAccount is null || !_foldersByAccount.TryGetValue(accountId, out var folders)) return; + + if (!string.IsNullOrWhiteSpace(rule.MoveToFolderId)) + rule.MoveToFolderName = folders.FirstOrDefault(f => f.FullName == rule.MoveToFolderId)?.DisplayName; + if (!string.IsNullOrWhiteSpace(rule.CopyToFolderId)) + rule.CopyToFolderName = folders.FirstOrDefault(f => f.FullName == rule.CopyToFolderId)?.DisplayName; + } + // ── State ─────────────────────────────────────────────────────────────── public ObservableCollection Rules { get; } = []; @@ -112,14 +135,22 @@ private async Task RefreshAsync(CancellationToken ct) var previouslySelected = SelectedRule?.Id; Rules.Clear(); - foreach (var r in rules) Rules.Add(r); + foreach (var r in rules) + { + ResolveFolderNames(r); + Rules.Add(r); + } SelectedRule = Rules.FirstOrDefault(r => r.Id == previouslySelected) ?? Rules.FirstOrDefault(); var disabled = Rules.Count(r => !r.IsEnabled); + var notEditable = Rules.Count(r => !r.IsFullyEditable); StatusText = Rules.Count == 0 ? "No server rules." - : $"{Rules.Count} rule{(Rules.Count == 1 ? "" : "s")}" + (disabled > 0 ? $", {disabled} disabled." : "."); + : $"{Rules.Count} rule{(Rules.Count == 1 ? "" : "s")}" + + (disabled > 0 ? $", {disabled} disabled" : "") + + (notEditable > 0 ? $", {notEditable} not editable in QuickMail" : "") + + "."; Announce(StatusText, AnnouncementCategory.Status); } catch (ServerRuleConsentRequiredException ex) diff --git a/QuickMail/Views/MainWindow.xaml.cs b/QuickMail/Views/MainWindow.xaml.cs index 43815ada..48c8e0d7 100644 --- a/QuickMail/Views/MainWindow.xaml.cs +++ b/QuickMail/Views/MainWindow.xaml.cs @@ -195,6 +195,7 @@ public partial class MainWindow : Window private readonly ILocalStoreService _localStore; private readonly IViewService _viewService; private readonly IRuleService _ruleService; + private readonly IServerRuleService? _serverRuleService; private readonly ITemplateService _templateService; private readonly IFlagService? _flagService; private readonly ICustomDictionaryService? _customDictionary; @@ -258,7 +259,8 @@ public MainWindow( IBugReportService? bugReportService = null, INotificationService? notificationService = null, IContactSyncService? contactSyncService = null, - IGraphCalendarSyncService? graphCalendarSyncService = null) + IGraphCalendarSyncService? graphCalendarSyncService = null, + IServerRuleService? serverRuleService = null) { _vm = vm; _notificationService = notificationService; @@ -271,6 +273,7 @@ public MainWindow( _contactService = contactService; _contactSyncService = contactSyncService; _graphCalendarSyncService = graphCalendarSyncService; + _serverRuleService = serverRuleService; _configService = configService; _localStore = localStore; _viewService = viewService; @@ -5846,7 +5849,13 @@ private void OpenRulesManager(MailRule? template = null) // Manager". Leaving it unowned makes it an independent peer that reads its own title — the // same fix compose and standalone message windows use. Unowned windows aren't auto-closed // with the main window, so it's tracked in _rulesWindow and closed in OnClosed. - var dialog = new RulesManagerWindow(rulesVm, accounts, _vm.CachedFolders); + // Read-only server-rules peek (#333): when a Graph account exists, surface its Exchange + // messageRules in the Rules Manager so the user can see them. Editing lands in a follow-up. + ServerRulesViewModel? serverRulesVm = null; + if (_serverRuleService != null && accounts.Any(a => a.BackendKind == BackendKind.MicrosoftGraph)) + serverRulesVm = new ServerRulesViewModel(_serverRuleService, accounts, _vm.CachedFolders); + + var dialog = new RulesManagerWindow(rulesVm, accounts, _vm.CachedFolders, serverRulesVm); _rulesWindow = dialog; // Modeless (.Show, NOT .ShowDialog). Opening this window modally over MainWindow's diff --git a/QuickMail/Views/RulesManagerWindow.xaml b/QuickMail/Views/RulesManagerWindow.xaml index 087c55f0..78f93c45 100644 --- a/QuickMail/Views/RulesManagerWindow.xaml +++ b/QuickMail/Views/RulesManagerWindow.xaml @@ -36,6 +36,7 @@ + @@ -278,10 +279,62 @@ + + + + + + + + + + + + + + + + + + + + + + + + + - + - diff --git a/QuickMail/Views/RulesManagerWindow.xaml.cs b/QuickMail/Views/RulesManagerWindow.xaml.cs index ff7b2e82..20e8d407 100644 --- a/QuickMail/Views/RulesManagerWindow.xaml.cs +++ b/QuickMail/Views/RulesManagerWindow.xaml.cs @@ -6,6 +6,7 @@ using System.Windows.Controls; using System.Windows.Data; using System.Windows.Input; +using System.Windows.Media; using QuickMail.Models; using QuickMail.ViewModels; @@ -27,15 +28,19 @@ public partial class RulesManagerWindow : Window private readonly IEnumerable _accounts; private readonly IReadOnlyDictionary> _cachedFolders; + private readonly ServerRulesViewModel? _serverRulesVm; + public RulesManagerWindow( RulesManagerViewModel vm, IEnumerable accounts, - IReadOnlyDictionary> cachedFolders) + IReadOnlyDictionary> cachedFolders, + ServerRulesViewModel? serverRulesVm = null) { InitializeComponent(); _vm = vm; _accounts = accounts; _cachedFolders = cachedFolders; + _serverRulesVm = serverRulesVm; DataContext = vm; // Wire VM events @@ -44,10 +49,78 @@ public RulesManagerWindow( vm.AnnouncementRequested += OnAnnouncementRequested; vm.PickFolderRequested += OnPickFolderRequested; - // Focus the first rule in the list on open (issue #348). Focusing the ListBox alone does - // not reliably move keyboard focus onto an item for some screen readers, so focus the - // first item's container directly. - Loaded += (_, _) => FocusFirstRule(); + // Read-only server-rules section (#333): a distinct sub-tree with its own DataContext so its + // bindings resolve against the ServerRulesViewModel rather than the client-rules VM. Hidden + // entirely when there's no Graph account. + if (_serverRulesVm is not null) + { + ServerRulesSection.DataContext = _serverRulesVm; + ServerRulesSection.Visibility = Visibility.Visible; + _serverRulesVm.AnnouncementRequested += OnAnnouncementRequested; + _serverRulesVm.WriteBlockedByPermission += OnServerRulesPermissionMessage; + // Load the account's server rules, THEN decide focus — landing focus on the list that + // actually holds the user's rules (they may have only server rules, no client rules). + Loaded += async (_, _) => + { + await _serverRulesVm.RefreshCommand.ExecuteAsync(null); + FocusInitialList(); + }; + } + else + { + // No server rules in play: original behaviour — focus the client rule list on open (#348). + Loaded += (_, _) => FocusFirstRule(); + } + } + + /// + /// Lands focus on whichever list has content. A Graph account with no client rules would + /// otherwise open onto the empty client list and sound empty while the user's (server) rules sit + /// in the section below. + /// + private void FocusInitialList() + { + if (RuleListBox.Items.Count > 0 || ServerRulesListBox.Items.Count == 0) + { + FocusFirstRule(); + return; + } + + ServerRulesListBox.SelectedIndex = 0; + ServerRulesListBox.UpdateLayout(); + if (ServerRulesListBox.ItemContainerGenerator.ContainerFromIndex(0) is ListBoxItem item) + item.Focus(); + else + ServerRulesListBox.Focus(); + } + + private void OnServerRulesPermissionMessage(string message) + => AccessibilityHelper.Announce(this, message, category: AnnouncementCategory.Hint); + + /// Moves keyboard focus to the next (or previous) window pane for F6 / Shift+F6. + private void CyclePane(bool forward) + { + // Only include panes that are actually present/visible. + var stops = new List { RuleListBox }; + if (_serverRulesVm is not null && ServerRulesSection.Visibility == Visibility.Visible) + { + stops.Add(ServerRulesListBox); + stops.Add(ServerRulesDetailBox); + stops.Add(ServerRulesStatusText); + } + stops.Add(MainStatusText); + + // Find where focus currently sits (walk up from the focused element to a known pane). + var current = -1; + for (var node = Keyboard.FocusedElement as DependencyObject; node is not null; node = VisualTreeHelper.GetParent(node)) + { + if (node is UIElement el && (current = stops.IndexOf(el)) >= 0) break; + } + + var next = current < 0 + ? 0 + : (current + (forward ? 1 : stops.Count - 1)) % stops.Count; + stops[next].Focus(); } private void FocusFirstRule() @@ -133,6 +206,16 @@ protected override void OnPreviewKeyDown(KeyEventArgs e) // so the Close button's IsCancel="True" no longer closes it on Escape — wire that // explicitly here. Step aside when an open combo dropdown needs Escape to dismiss // itself, so we don't steal it (matches ComposeWindow's guard). + // F6 / Shift+F6 cycle between the window's panes (New Window Checklist). Stops are the client + // rule list, the server-rules list and detail (when shown), and the status line — so a + // keyboard/screen-reader user can reach every region, including the status to re-read counts. + if (e.Key == Key.F6) + { + CyclePane(forward: (Keyboard.Modifiers & ModifierKeys.Shift) == 0); + e.Handled = true; + return; + } + if (e.Key == Key.Escape) { if (AccountScopeCombo.IsDropDownOpen || ActionCombo.IsDropDownOpen) @@ -149,6 +232,11 @@ protected override void OnClosed(EventArgs e) _vm.ConfirmDeleteRequested -= OnConfirmDeleteRequested; _vm.AnnouncementRequested -= OnAnnouncementRequested; _vm.PickFolderRequested -= OnPickFolderRequested; + if (_serverRulesVm is not null) + { + _serverRulesVm.AnnouncementRequested -= OnAnnouncementRequested; + _serverRulesVm.WriteBlockedByPermission -= OnServerRulesPermissionMessage; + } base.OnClosed(e); } } From d65173053e66b0fc39a092c979fcaf94a3ef6ab7 Mon Sep 17 00:00:00 2001 From: Timothy Spaulding Date: Fri, 24 Jul 2026 22:06:42 -0400 Subject: [PATCH 2/6] Gate the server-rules UI behind a ServerRules feature flag (default off) (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes the in-progress server-rules feature invisible to the public while it's built out, so intermediate merges (this read-only view, then editing, then the unified window) can't expose a half-built experience — regardless of when they merge. Mirrors the GraphBackend gate. - New FeatureFlag.ServerRules, default false in ConfigFeatureGate.Defaults. - MainWindow.OpenRulesManager only builds the ServerRulesViewModel when the gate is enabled (and a Graph account exists), so the Rules Manager looks exactly as it does today for everyone else. - Enable for dev/testing with --feature ServerRules or config.ini [features] ServerRules=true. Flip the default in a joint-decision PR once create/edit/delete and the unified per-account window are complete. Co-Authored-By: Claude Opus 4.8 --- QuickMail/Models/FeatureFlag.cs | 9 +++++++++ QuickMail/Services/ConfigFeatureGate.cs | 1 + QuickMail/Views/MainWindow.xaml.cs | 4 +++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/QuickMail/Models/FeatureFlag.cs b/QuickMail/Models/FeatureFlag.cs index f7c87819..1688c85e 100644 --- a/QuickMail/Models/FeatureFlag.cs +++ b/QuickMail/Models/FeatureFlag.cs @@ -18,4 +18,13 @@ public enum FeatureFlag /// Default: true. Disable in config.ini with GoogleAuth=false under [features]. /// GoogleAuth, + + /// + /// Shows server-side (Exchange/Graph) Inbox rules in the Rules Manager for Microsoft 365 + /// accounts (#333). Default: false while the feature is built out — enable at launch with + /// --feature ServerRules or in config.ini with ServerRules=true under [features]. Flip the + /// default to true via a joint-decision PR once create/edit/delete and the unified per-account + /// window are complete. + /// + ServerRules, } diff --git a/QuickMail/Services/ConfigFeatureGate.cs b/QuickMail/Services/ConfigFeatureGate.cs index 9684a76f..13c7dd80 100644 --- a/QuickMail/Services/ConfigFeatureGate.cs +++ b/QuickMail/Services/ConfigFeatureGate.cs @@ -17,6 +17,7 @@ public class ConfigFeatureGate : IFeatureGate { [FeatureFlag.GraphBackend] = true, [FeatureFlag.GoogleAuth] = true, + [FeatureFlag.ServerRules] = false, // off until server-rule editing + unified window ship (#333) }; private readonly Dictionary _configFlags; diff --git a/QuickMail/Views/MainWindow.xaml.cs b/QuickMail/Views/MainWindow.xaml.cs index 48c8e0d7..356f8490 100644 --- a/QuickMail/Views/MainWindow.xaml.cs +++ b/QuickMail/Views/MainWindow.xaml.cs @@ -5852,7 +5852,9 @@ private void OpenRulesManager(MailRule? template = null) // Read-only server-rules peek (#333): when a Graph account exists, surface its Exchange // messageRules in the Rules Manager so the user can see them. Editing lands in a follow-up. ServerRulesViewModel? serverRulesVm = null; - if (_serverRuleService != null && accounts.Any(a => a.BackendKind == BackendKind.MicrosoftGraph)) + if (_serverRuleService != null + && _featureGate.IsEnabled(FeatureFlag.ServerRules) + && accounts.Any(a => a.BackendKind == BackendKind.MicrosoftGraph)) serverRulesVm = new ServerRulesViewModel(_serverRuleService, accounts, _vm.CachedFolders); var dialog = new RulesManagerWindow(rulesVm, accounts, _vm.CachedFolders, serverRulesVm); From bd64379b495211e7d3f57821bf1b6e923dd2fe14 Mon Sep 17 00:00:00 2001 From: Timothy Spaulding Date: Fri, 24 Jul 2026 22:11:09 -0400 Subject: [PATCH 3/6] Server rule editor VM: cover bodyContains, sentToAddresses, copyToFolder (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness prerequisite for editing. The mapper/model gained bodyContains, sentToAddresses, and copyToFolder (so real O365 rules are fully representable), but the editor VM didn't know those fields — so editing such a rule would have assembled a ToModel() without them and the PATCH would silently drop them (the exact §16 data-loss the design guards against). ServerRuleEditorViewModel now reads them in ForEdit, exposes fields (incl. a separate CopyToFolder + PickCopyFolder), writes them in ToModel, and validates the copy-folder target. So every condition/action the model can represent also round-trips through the editor. Tests: extended the round-trip test to assert bodyContains/sentToAddresses/ copyToFolder survive an edit, plus copy-folder validation. Co-Authored-By: Claude Opus 4.8 --- QuickMail.Tests/ServerRulesViewModelTests.cs | 26 ++++++++++++-- .../ViewModels/ServerRuleEditorViewModel.cs | 34 +++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/QuickMail.Tests/ServerRulesViewModelTests.cs b/QuickMail.Tests/ServerRulesViewModelTests.cs index 5066612e..729157b8 100644 --- a/QuickMail.Tests/ServerRulesViewModelTests.cs +++ b/QuickMail.Tests/ServerRulesViewModelTests.cs @@ -415,8 +415,10 @@ public void Editor_Save_RaisesSavedWithAssembledRule_AndCloses() } [Fact] - public void Editor_ForEdit_RoundTripsFields() + public void Editor_ForEdit_RoundTripsFields_IncludingBodyContainsSentToAndCopy() { + // Covers the fields added in #333 (bodyContains, sentToAddresses, copyToFolder) — an edit + // must carry them through, not drop them (the §16 data-loss trap). var original = new ServerRuleModel { Id = "r1", @@ -425,28 +427,46 @@ public void Editor_ForEdit_RoundTripsFields() IsEnabled = false, SenderContains = "boss", SubjectContains = "urgent", + BodyContains = "invoice", + SentToAddresses = ["team@contoso.com", "ops@contoso.com"], SentOnlyToMe = true, Importance = "high", MoveToFolderId = "folder-1", MoveToFolderName = "Priority", + CopyToFolderId = "folder-2", + CopyToFolderName = "Backups", StopProcessingRules = true, IsFullyEditable = true, }; - var editor = ServerRuleEditorViewModel.ForEdit(original); - var result = editor.ToModel(); + var result = ServerRuleEditorViewModel.ForEdit(original).ToModel(); Assert.Equal("r1", result.Id); Assert.Equal(3, result.Sequence); Assert.Equal("Alpha", result.DisplayName); Assert.False(result.IsEnabled); Assert.Equal("boss", result.SenderContains); + Assert.Equal("urgent", result.SubjectContains); + Assert.Equal("invoice", result.BodyContains); + Assert.Equal(["team@contoso.com", "ops@contoso.com"], result.SentToAddresses); Assert.True(result.SentOnlyToMe); Assert.Equal("high", result.Importance); Assert.Equal("folder-1", result.MoveToFolderId); + Assert.Equal("folder-2", result.CopyToFolderId); Assert.True(result.StopProcessingRules); } + [Fact] + public void Editor_CopyToFolderWithoutAFolder_IsInvalid() + { + var editor = ServerRuleEditorViewModel.ForNew(); + editor.Name = "Copier"; + editor.CopyToFolder = true; // checked, but no folder picked + + Assert.False(editor.Validate()); + Assert.Contains("folder", editor.FolderError, StringComparison.OrdinalIgnoreCase); + } + [Fact] public void ImportanceOption_AnnouncesDisplayName_NotTypeName() { diff --git a/QuickMail/ViewModels/ServerRuleEditorViewModel.cs b/QuickMail/ViewModels/ServerRuleEditorViewModel.cs index 7a4f562e..cbfea38c 100644 --- a/QuickMail/ViewModels/ServerRuleEditorViewModel.cs +++ b/QuickMail/ViewModels/ServerRuleEditorViewModel.cs @@ -59,8 +59,10 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) SenderContains = rule.SenderContains ?? string.Empty, FromAddresses = string.Join(", ", rule.FromAddresses), + SentToAddresses = string.Join(", ", rule.SentToAddresses), SubjectContains = rule.SubjectContains ?? string.Empty, BodyOrSubjectContains = rule.BodyOrSubjectContains ?? string.Empty, + BodyContains = rule.BodyContains ?? string.Empty, SentToMe = rule.SentToMe, SentOnlyToMe = rule.SentOnlyToMe, HasAttachments = rule.HasAttachments, @@ -68,6 +70,9 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) MoveToFolder = !string.IsNullOrWhiteSpace(rule.MoveToFolderId), MoveToFolderId = rule.MoveToFolderId, MoveToFolderName = rule.MoveToFolderName, + CopyToFolder = !string.IsNullOrWhiteSpace(rule.CopyToFolderId), + CopyToFolderId = rule.CopyToFolderId, + CopyToFolderName = rule.CopyToFolderName, MarkAsRead = rule.MarkAsRead, Delete = rule.Delete, ForwardTo = string.Join(", ", rule.ForwardTo), @@ -89,8 +94,10 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) // Conditions [ObservableProperty] private string _senderContains = string.Empty; [ObservableProperty] private string _fromAddresses = string.Empty; + [ObservableProperty] private string _sentToAddresses = string.Empty; [ObservableProperty] private string _subjectContains = string.Empty; [ObservableProperty] private string _bodyOrSubjectContains = string.Empty; + [ObservableProperty] private string _bodyContains = string.Empty; [ObservableProperty] private bool _sentToMe; [ObservableProperty] private bool _sentOnlyToMe; [ObservableProperty] private bool _hasAttachments; @@ -102,6 +109,11 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) private bool _moveToFolder; [ObservableProperty] private string? _moveToFolderId; [ObservableProperty] private string? _moveToFolderName; + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(IsCopyToFolderSelected))] + private bool _copyToFolder; + [ObservableProperty] private string? _copyToFolderId; + [ObservableProperty] private string? _copyToFolderName; [ObservableProperty] private bool _markAsRead; [ObservableProperty] private ImportanceOption _selectedMarkImportance = ImportanceOptions[0]; [ObservableProperty] private bool _delete; @@ -109,6 +121,7 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) [ObservableProperty] private bool _stopProcessingRules; public bool IsMoveToFolderSelected => MoveToFolder; + public bool IsCopyToFolderSelected => CopyToFolder; // Validation surfaces [ObservableProperty] private string _nameError = string.Empty; @@ -136,6 +149,16 @@ private void PickFolder() FolderError = string.Empty; } + [RelayCommand] + private void PickCopyFolder() + { + if (PickFolderRequested?.Invoke() is not { } picked) return; + CopyToFolderId = picked.Id; + CopyToFolderName = picked.Name; + CopyToFolder = true; + FolderError = string.Empty; + } + [RelayCommand] private void Save() { @@ -158,8 +181,10 @@ private void Save() SenderContains = Blank(SenderContains), FromAddresses = SplitAddresses(FromAddresses), + SentToAddresses = SplitAddresses(SentToAddresses), SubjectContains = Blank(SubjectContains), BodyOrSubjectContains = Blank(BodyOrSubjectContains), + BodyContains = Blank(BodyContains), SentToMe = SentToMe, SentOnlyToMe = SentOnlyToMe, HasAttachments = HasAttachments, @@ -167,6 +192,8 @@ private void Save() MoveToFolderId = MoveToFolder ? MoveToFolderId : null, MoveToFolderName = MoveToFolder ? MoveToFolderName : null, + CopyToFolderId = CopyToFolder ? CopyToFolderId : null, + CopyToFolderName = CopyToFolder ? CopyToFolderName : null, MarkAsRead = MarkAsRead, MarkImportance = SelectedMarkImportance?.Value, Delete = Delete, @@ -198,6 +225,12 @@ public bool Validate() valid = false; } + if (CopyToFolder && string.IsNullOrWhiteSpace(CopyToFolderId)) + { + FolderError = "Choose a folder for the Copy to folder action."; + valid = false; + } + if (!HasAnyAction()) { ActionsError = "Choose at least one action."; @@ -215,6 +248,7 @@ public bool Validate() private bool HasAnyAction() => (MoveToFolder && !string.IsNullOrWhiteSpace(MoveToFolderId)) + || (CopyToFolder && !string.IsNullOrWhiteSpace(CopyToFolderId)) || MarkAsRead || Delete || StopProcessingRules From b4d2472a3b08b995c2e8be54bba1c9324047b07d Mon Sep 17 00:00:00 2001 From: Timothy Spaulding Date: Sun, 26 Jul 2026 10:42:27 -0400 Subject: [PATCH 4/6] Server rules: full editing, account picker, and round-trip fixes (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feature remains gated off (FeatureFlag.ServerRules, default false). This is a checkpoint of the server-rules-window work verified end-to-end against a live M365 mailbox (create → server rule fires). Editing - New modeless ServerRuleEditorWindow (create/edit) — modeless per the editable-text-over-WebView2 rule; Escape/Cancel wired explicitly. - Editor laid out as Common + a collapsible Advanced section; Advanced auto-expands when editing a rule that already uses an advanced field. - Support bodyContains, sentToAddresses, copyToFolder (covers every predicate in the test mailbox, so 0 rules are flagged un-editable). Reorder / read-only - Minimal-diff reorder: only PATCH the rules whose sequence actually changes, so an unrelated server-read-only rule (e.g. an Outlook-created one) no longer 400s and poisons an otherwise valid move. - Read-only rules disable Edit/Delete/Enable-Disable/Move via CanExecute (buttons and context-menu items), rather than being pressable-but-silent — the user runs with announcements off, so a disabled control is clearer. Keyboard / focus / account - Space = enable/disable, Delete = delete, Enter = edit on the server list (window-scoped keys, honouring each command's CanExecute). - Account picker added to the server section, seeded to the account the user is currently in (falls back to the first Graph account on aggregate views). - Focus returns to the selected/new rule after writes; container-based focus with virtualization off so arrow keys stay in the list. Round-trip correctness - New rules get a valid 1-based sequence (Graph rejects sequence 0). - Save reports success/failure back to the editor: it stays open and shows the error on failure instead of closing and losing the form. - Guard MainWindow's OnActivated focus-restore with IsActive so a transient activation while the editor closes can't steal focus back to the message list. Design - Spec §20: unified New/Create with auto-classify (server vs client) — the agreed direction for the remaining layout-unification work. Co-Authored-By: Claude Opus 4.8 --- .../GraphServerRuleServiceTests.cs | 24 ++- QuickMail.Tests/ServerRulesViewModelTests.cs | 180 +++++++++++++++--- QuickMail.Tests/UnitTest1.cs | 12 ++ QuickMail/Models/ServerRuleModel.cs | 16 +- QuickMail/Services/GraphServerRuleService.cs | 45 +++-- QuickMail/Services/IServerRuleService.cs | 8 +- .../ViewModels/ServerRuleEditorViewModel.cs | 51 ++++- QuickMail/ViewModels/ServerRulesViewModel.cs | 169 +++++++++------- QuickMail/Views/MainWindow.xaml.cs | 14 +- QuickMail/Views/RulesManagerWindow.xaml | 53 +++++- QuickMail/Views/RulesManagerWindow.xaml.cs | 85 +++++++++ QuickMail/Views/ServerRuleEditorWindow.xaml | 155 +++++++++++++++ .../Views/ServerRuleEditorWindow.xaml.cs | 85 +++++++++ docs/planning/server-rules-pm-dev-spec.md | 96 ++++++++++ 14 files changed, 870 insertions(+), 123 deletions(-) create mode 100644 QuickMail/Views/ServerRuleEditorWindow.xaml create mode 100644 QuickMail/Views/ServerRuleEditorWindow.xaml.cs diff --git a/QuickMail.Tests/GraphServerRuleServiceTests.cs b/QuickMail.Tests/GraphServerRuleServiceTests.cs index 62f2d892..dae19511 100644 --- a/QuickMail.Tests/GraphServerRuleServiceTests.cs +++ b/QuickMail.Tests/GraphServerRuleServiceTests.cs @@ -422,20 +422,30 @@ public async Task SetEnabled_SendsOnlyIsEnabled_SoOtherFieldsSurvive() } [Fact] - public async Task Reorder_AssignsSequentialPositions() + public async Task Reorder_PatchesOnlyChangedRules_LeavingUntouchedRulesAlone() { - var handler = new RecordingHandler(Json("{}"), Json("{}"), Json("{}")); + // Rules a=1, b=2, c=3. Swap the first two → [b, a, c]. Only b and a change position; + // c keeps sequence 3 and must NOT be PATCHed. This is what stops a server-protected rule + // elsewhere in the list (which 400s on any PATCH) from poisoning an unrelated move. + var a = new ServerRuleModel { Id = "a", Sequence = 1 }; + var b = new ServerRuleModel { Id = "b", Sequence = 2 }; + var c = new ServerRuleModel { Id = "c", Sequence = 3 }; + var handler = new RecordingHandler(Json("{}"), Json("{}")); - await Service(handler).ReorderAsync(_accountId, ["c", "a", "b"]); + await Service(handler).ReorderAsync(_accountId, new[] { b, a, c }); - Assert.Equal(3, handler.Urls.Count); + Assert.Equal(2, handler.Urls.Count); Assert.All(handler.Methods, m => Assert.Equal("PATCH", m)); - Assert.EndsWith("/messageRules/c", handler.Urls[0]); + Assert.EndsWith("/messageRules/b", handler.Urls[0]); Assert.Contains("\"sequence\":1", handler.Bodies[0]!); Assert.EndsWith("/messageRules/a", handler.Urls[1]); Assert.Contains("\"sequence\":2", handler.Bodies[1]!); - Assert.EndsWith("/messageRules/b", handler.Urls[2]); - Assert.Contains("\"sequence\":3", handler.Bodies[2]!); + Assert.DoesNotContain(handler.Urls, u => u.EndsWith("/messageRules/c")); + + // Local models reflect the sequence values the server was told. + Assert.Equal(1, b.Sequence); + Assert.Equal(2, a.Sequence); + Assert.Equal(3, c.Sequence); } [Fact] diff --git a/QuickMail.Tests/ServerRulesViewModelTests.cs b/QuickMail.Tests/ServerRulesViewModelTests.cs index 729157b8..c1019e9b 100644 --- a/QuickMail.Tests/ServerRulesViewModelTests.cs +++ b/QuickMail.Tests/ServerRulesViewModelTests.cs @@ -62,10 +62,10 @@ public Task SetEnabledAsync(Guid accountId, string ruleId, bool enabled, Cancell return Task.CompletedTask; } - public Task ReorderAsync(Guid accountId, IReadOnlyList ruleIdsInOrder, CancellationToken ct = default) + public Task ReorderAsync(Guid accountId, IReadOnlyList rulesInOrder, CancellationToken ct = default) { Calls.Add("reorder"); - LastReorder = ruleIdsInOrder; + LastReorder = rulesInOrder.Select(r => r.Id).ToList(); if (ThrowOnWrite is not null) throw ThrowOnWrite; return Task.CompletedTask; } @@ -89,6 +89,21 @@ public Task DeleteAsync(Guid accountId, string ruleId, CancellationToken ct = de private ServerRulesViewModel Vm(FakeServerRuleService svc, params AccountModel[] accounts) => new(svc, accounts.Length > 0 ? accounts : [GraphAccount()]); + [Fact] + public void Ctor_DefaultsToPreferredAccount_WhenProvided() + { + var first = new AccountModel { Id = Guid.NewGuid(), BackendKind = BackendKind.MicrosoftGraph, Username = "a@x.com", AccountName = "A" }; + var second = new AccountModel { Id = Guid.NewGuid(), BackendKind = BackendKind.MicrosoftGraph, Username = "b@x.com", AccountName = "B" }; + + // Opening from the second account's inbox should land on the second account, not the first. + var vm = new ServerRulesViewModel(new FakeServerRuleService(), [first, second], null, second.Id); + Assert.Equal(second.Id, vm.SelectedAccount?.Id); + + // No current-account context (e.g. an aggregate view) → fall back to the first account. + var fallback = new ServerRulesViewModel(new FakeServerRuleService(), [first, second], null, null); + Assert.Equal(first.Id, fallback.SelectedAccount?.Id); + } + private static ServerRuleModel Rule(string id, string name, bool enabled = true, bool editable = true, bool readOnly = false) => new() { @@ -162,19 +177,32 @@ public async Task CanEditSelected_ReflectsEditabilityAndReadOnly(bool editable, } [Fact] - public async Task EditRule_OnNonEditableRule_DoesNotOpenEditor_AndExplainsWhy() + public async Task EditRule_OnNonEditableRule_IsDisabled_AndDoesNotOpenEditor() { var svc = new FakeServerRuleService { Stored = [Rule("a", "Complex", editable: false)] }; var vm = Vm(svc); await vm.RefreshCommand.ExecuteAsync(null); + // Edit is disabled (not pressable) for a rule we can't fully represent — the user runs with + // announcements off, so a disabled control is clearer than a pressable one that silently fails. + Assert.False(vm.EditRuleCommand.CanExecute(null)); + var opened = false; vm.EditorRequested += _ => opened = true; + vm.EditRuleCommand.Execute(null); // force-invoke: the defensive guard still blocks it + Assert.False(opened); + } - vm.EditRuleCommand.Execute(null); + [Fact] + public async Task ToggleAndDelete_AreDisabled_ForServerReadOnlyRule() + { + var svc = new FakeServerRuleService { Stored = [Rule("ro", "Protected", readOnly: true)] }; + var vm = Vm(svc); + await vm.RefreshCommand.ExecuteAsync(null); - Assert.False(opened); - Assert.Contains("Outlook", vm.StatusText); + Assert.False(vm.ToggleEnabledCommand.CanExecute(null)); + Assert.False(vm.DeleteRuleCommand.CanExecute(null)); + Assert.False(vm.EditRuleCommand.CanExecute(null)); } [Fact] @@ -209,6 +237,28 @@ public void CreateRule_OpensEmptyEditor() Assert.Equal(string.Empty, editor.Name); } + [Fact] + public async Task CreateRule_OnEmptyAccount_AssignsSequence1_NotZero() + { + // Graph rejects sequence 0 with MessageRuleValidationError; a new rule must get a 1-based + // sequence. On an account with no rules yet, the first new rule is sequence 1. + var svc = new FakeServerRuleService(); + var vm = Vm(svc); + await vm.RefreshCommand.ExecuteAsync(null); + + ServerRuleEditorViewModel? editor = null; + vm.EditorRequested += e => editor = e; + vm.CreateRuleCommand.Execute(null); + + editor!.Name = "First rule"; + editor.MarkAsRead = true; + await editor.SaveCommand.ExecuteAsync(null); + + Assert.Contains("create", svc.Calls); + Assert.Single(vm.Rules); + Assert.Equal(1, vm.Rules[0].Sequence); + } + // ── Toggle / delete / reorder ─────────────────────────────────────────────── [Fact] @@ -225,26 +275,27 @@ public async Task ToggleEnabled_FlipsStateAndCallsService() } [Fact] - public async Task ToggleEnabled_RaisesCollectionChange_SoTheRowTextRefreshes() + public async Task ToggleEnabled_UpdatesRowText_InPlace_ViaNotification() { - // A row's announced text comes from ServerRuleModel.ToString(), which a ListView evaluates - // once per container. The model raises no change notification, so without re-assigning the - // slot the row would keep announcing "enabled" after the user disabled it. + // The row's accessible name is bound to ServerRuleModel.RowText, which change-notifies when + // IsEnabled flips. So a toggle re-announces the new state WITHOUT re-inserting the row (which + // would disturb a screen reader's focus). Re-assigning the same object into the collection + // was a no-op for the WPF generator, which is why the row didn't refresh before. var svc = new FakeServerRuleService { Stored = [Rule("a", "Alpha", enabled: true)] }; var vm = Vm(svc); await vm.RefreshCommand.ExecuteAsync(null); - var replaced = false; - vm.Rules.CollectionChanged += (_, e) => - { - if (e.Action == System.Collections.Specialized.NotifyCollectionChangedAction.Replace) replaced = true; - }; + var rule = vm.Rules[0]; + var rowTextChanged = false; + rule.PropertyChanged += (_, e) => { if (e.PropertyName == nameof(ServerRuleModel.RowText)) rowTextChanged = true; }; await vm.ToggleEnabledCommand.ExecuteAsync(null); - Assert.True(replaced); - Assert.Contains("disabled", vm.Rules[0].ToString()); - Assert.Same(vm.Rules[0], vm.SelectedRule); // selection survives the replace + Assert.False(rule.IsEnabled); + Assert.True(rowTextChanged); // UIA gets the change the screen reader needs + Assert.Contains("disabled", rule.RowText); + Assert.Same(vm.Rules[0], vm.SelectedRule); // no re-insert; selection undisturbed + Assert.Equal("Enable", vm.ToggleEnabledLabel); // button now offers the opposite action } [Fact] @@ -289,6 +340,50 @@ public async Task DeleteRule_Confirmed_RemovesAndMovesSelection() Assert.Equal("b", vm.SelectedRule!.Id); } + [Fact] + public async Task MoveUpDown_AreDisabled_AtTheEnds() + { + var svc = new FakeServerRuleService { Stored = [Rule("a", "Alpha"), Rule("b", "Beta"), Rule("c", "Gamma")] }; + var vm = Vm(svc); + await vm.RefreshCommand.ExecuteAsync(null); + + // First rule selected: can't move up. + Assert.False(vm.CanMoveUp); + Assert.True(vm.CanMoveDown); + Assert.False(vm.MoveUpCommand.CanExecute(null)); + Assert.True(vm.MoveDownCommand.CanExecute(null)); + + // Last rule selected: can't move down. + vm.SelectedRule = vm.Rules[2]; + Assert.True(vm.CanMoveUp); + Assert.False(vm.CanMoveDown); + Assert.False(vm.MoveDownCommand.CanExecute(null)); + + // Middle: both. + vm.SelectedRule = vm.Rules[1]; + Assert.True(vm.CanMoveUp); + Assert.True(vm.CanMoveDown); + } + + [Fact] + public async Task MoveUpDown_AreDisabled_ForServerReadOnlyRule() + { + // A read-only rule (e.g. "Delete Pokémon messages") can't be re-sequenced by the API; Move + // must be gated like Edit/Delete even when it's in the middle of the list. + var svc = new FakeServerRuleService + { + Stored = [Rule("a", "Alpha"), Rule("ro", "Protected", readOnly: true), Rule("c", "Gamma")], + }; + var vm = Vm(svc); + await vm.RefreshCommand.ExecuteAsync(null); + + vm.SelectedRule = vm.Rules[1]; // the read-only rule, mid-list + Assert.False(vm.CanMoveUp); + Assert.False(vm.CanMoveDown); + Assert.False(vm.MoveUpCommand.CanExecute(null)); + Assert.False(vm.MoveDownCommand.CanExecute(null)); + } + [Fact] public async Task MoveDown_ReordersAndSendsNewOrder() { @@ -300,7 +395,8 @@ public async Task MoveDown_ReordersAndSendsNewOrder() Assert.Equal(["b", "a"], vm.Rules.Select(r => r.Id)); Assert.Equal(["b", "a"], svc.LastReorder); - Assert.Equal([1, 2], vm.Rules.Select(r => r.Sequence)); + // Sequence-value reassignment is the service's job (verified in GraphServerRuleServiceTests); + // the VM only owns the visible order and the service call. } [Fact] @@ -391,7 +487,7 @@ public void Editor_MoveToFolderWithoutAFolder_IsInvalid() } [Fact] - public void Editor_Save_RaisesSavedWithAssembledRule_AndCloses() + public async Task Editor_Save_RaisesSavedWithAssembledRule_AndCloses() { var editor = ServerRuleEditorViewModel.ForNew(); editor.Name = " Newsletters "; @@ -401,10 +497,10 @@ public void Editor_Save_RaisesSavedWithAssembledRule_AndCloses() ServerRuleModel? saved = null; var closed = false; - editor.Saved += r => saved = r; + editor.Saved += r => { saved = r; return Task.FromResult(null); }; // null = success editor.CloseRequested += () => closed = true; - editor.SaveCommand.Execute(null); + await editor.SaveCommand.ExecuteAsync(null); Assert.NotNull(saved); Assert.Equal("Newsletters", saved!.DisplayName); // trimmed @@ -414,6 +510,23 @@ public void Editor_Save_RaisesSavedWithAssembledRule_AndCloses() Assert.True(closed); } + [Fact] + public async Task Editor_Save_WhenOwnerReportsError_StaysOpen_AndShowsError() + { + var editor = ServerRuleEditorViewModel.ForNew(); + editor.Name = "Nope"; + editor.MarkAsRead = true; + + var closed = false; + editor.Saved += _ => Task.FromResult("Graph rejected the rule."); // non-null = failure + editor.CloseRequested += () => closed = true; + + await editor.SaveCommand.ExecuteAsync(null); + + Assert.False(closed); // editor stays open on failure + Assert.Equal("Graph rejected the rule.", editor.SaveError); + } + [Fact] public void Editor_ForEdit_RoundTripsFields_IncludingBodyContainsSentToAndCopy() { @@ -456,6 +569,29 @@ public void Editor_ForEdit_RoundTripsFields_IncludingBodyContainsSentToAndCopy() Assert.True(result.StopProcessingRules); } + [Fact] + public void Editor_ForNew_LeavesAdvancedCollapsed() + => Assert.False(ServerRuleEditorViewModel.ForNew().IsAdvancedExpanded); + + [Fact] + public void Editor_ForEdit_ExpandsAdvanced_WhenRuleUsesAnAdvancedField() + { + // "Sender contains" lives in the Advanced section; editing a rule that uses it must open + // Advanced so the populated field isn't hidden. + var withAdvanced = ServerRuleEditorViewModel.ForEdit(new ServerRuleModel + { + Id = "r1", DisplayName = "Alpha", SenderContains = "boss", MarkAsRead = true, + }); + Assert.True(withAdvanced.IsAdvancedExpanded); + + // A rule using only common fields keeps Advanced collapsed. + var commonOnly = ServerRuleEditorViewModel.ForEdit(new ServerRuleModel + { + Id = "r2", DisplayName = "Beta", SubjectContains = "invoice", MarkAsRead = true, + }); + Assert.False(commonOnly.IsAdvancedExpanded); + } + [Fact] public void Editor_CopyToFolderWithoutAFolder_IsInvalid() { diff --git a/QuickMail.Tests/UnitTest1.cs b/QuickMail.Tests/UnitTest1.cs index b8161caa..c5ffded7 100644 --- a/QuickMail.Tests/UnitTest1.cs +++ b/QuickMail.Tests/UnitTest1.cs @@ -340,6 +340,18 @@ public void EventEditorWindow_XamlParsesWithoutException() window.Close(); } + [StaFact] + public void ServerRuleEditorWindow_XamlParsesWithoutException() + { + EnsureApplication(); + var window = new ServerRuleEditorWindow( + ServerRuleEditorViewModel.ForNew(), + new List(), + new Dictionary>()); + Assert.NotNull(window); + window.Close(); + } + [StaFact] public void GroupManagerWindow_XamlParsesWithoutException() { diff --git a/QuickMail/Models/ServerRuleModel.cs b/QuickMail/Models/ServerRuleModel.cs index 2ee28522..1064880d 100644 --- a/QuickMail/Models/ServerRuleModel.cs +++ b/QuickMail/Models/ServerRuleModel.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Text; using System.Text.Json; +using CommunityToolkit.Mvvm.ComponentModel; namespace QuickMail.Models; @@ -18,7 +19,7 @@ namespace QuickMail.Models; /// in Outlook (spec §16, the central correctness risk). /// /// -public sealed class ServerRuleModel +public sealed partial class ServerRuleModel : ObservableObject { public string Id { get; set; } = string.Empty; public string DisplayName { get; set; } = string.Empty; @@ -26,7 +27,18 @@ public sealed class ServerRuleModel /// Execution order on the server. Lower runs first. public int Sequence { get; set; } - public bool IsEnabled { get; set; } = true; + /// + /// Observable so a list row's announced text updates in place when the rule is toggled — without + /// re-inserting the item (which would disturb a screen reader's focus). is + /// what the list binds its accessible name to; notifying it fires the UIA change the reader needs. + /// + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(RowText))] + private bool _isEnabled = true; + + /// The list row's accessible/display text (same as ), as a + /// change-notifying property so a toggle re-announces the new state. + public string RowText => ToString(); /// Server-set: the rule cannot be modified (edit/delete blocked). public bool IsReadOnly { get; set; } diff --git a/QuickMail/Services/GraphServerRuleService.cs b/QuickMail/Services/GraphServerRuleService.cs index 569d99af..961d7c9a 100644 --- a/QuickMail/Services/GraphServerRuleService.cs +++ b/QuickMail/Services/GraphServerRuleService.cs @@ -55,6 +55,14 @@ public async Task> ListAsync(Guid accountId, Canc // values, which would put mail subjects/addresses in the log. foreach (var r in rules.Where(x => !x.IsFullyEditable)) LogService.Debug($"ServerRules: not-editable '{r.DisplayName}' unsupported=[{string.Join(", ", r.UnsupportedFields)}]"); + var disabled = rules.Where(r => !r.IsEnabled).Select(r => r.DisplayName).ToList(); + if (disabled.Count > 0) + LogService.Debug($"ServerRules: disabled rules: {string.Join("; ", disabled)}"); + // Server read-only rules are the ones the API refuses to move/edit/delete + // (ErrorNotSupportedMessageRule). Name them so a reorder/edit failure is easy to trace back. + var readOnly = rules.Where(r => r.IsReadOnly).Select(r => r.DisplayName).ToList(); + if (readOnly.Count > 0) + LogService.Debug($"ServerRules: read-only rules: {string.Join("; ", readOnly)}"); return rules; } @@ -101,28 +109,37 @@ public async Task SetEnabledAsync(Guid accountId, string ruleId, bool enabled, C LogService.Log($"ServerRules: {(enabled ? "enabled" : "disabled")} rule {ruleId} for {account.Username}"); } - public async Task ReorderAsync(Guid accountId, IReadOnlyList ruleIdsInOrder, CancellationToken ct = default) + public async Task ReorderAsync(Guid accountId, IReadOnlyList rulesInOrder, CancellationToken ct = default) { var account = Account(accountId); - // Sequence is 1-based on the server; assign positions in the given order. Only `sequence` is - // sent, so the rest of each rule is untouched. + // Reassign which rule holds which sequence value, in the new order — WITHOUT renumbering the + // whole list. We keep the existing set of server sequence values and only PATCH the rules + // whose value actually changes. This is critical: a mailbox commonly contains a rule the + // server refuses to modify ("ErrorNotSupportedMessageRule"). A full 1..N re-sequence PATCHes + // every rule and 400s the moment it reaches that one — poisoning an otherwise valid move of + // an unrelated rule. By touching only the rules whose position changed (two, for a single + // Move up/down), a protected rule elsewhere is never sent a PATCH. // - // NOT ATOMIC, and it can't be: Graph exposes no batch/transactional reorder, so this is N - // sequential PATCHes. A failure partway leaves the server partially reordered, and duplicate - // sequence values exist transiently mid-loop (Graph tolerates this — it resolves ordering on - // read). The caller rolls back its LOCAL order on failure, which does not undo PATCHes - // already applied server-side; the next refresh shows the server's true order. Accepted for - // v1: the blast radius is rule ordering, not rule content. - for (var i = 0; i < ruleIdsInOrder.Count; i++) + // Still not atomic (Graph has no batch reorder) and a transient duplicate sequence can exist + // mid-loop — Graph tolerates that and resolves ordering on read. If a PATCH fails, the caller + // rolls back its LOCAL order; the next refresh reflects the server's true state. + var sortedSequences = rulesInOrder.Select(r => r.Sequence).OrderBy(s => s).ToList(); + var patched = 0; + for (var i = 0; i < rulesInOrder.Count; i++) { ct.ThrowIfCancellationRequested(); - var body = new Dictionary { ["sequence"] = i + 1 }; - var id = ruleIdsInOrder[i]; - await GuardAsync(() => _client.PatchAsync(account, $"{RulesPath}/{Uri.EscapeDataString(id)}", body, ct)); + var rule = rulesInOrder[i]; + var target = sortedSequences[i]; + if (rule.Sequence == target) continue; // unchanged position → don't touch this rule + + var body = new Dictionary { ["sequence"] = target }; + await GuardAsync(() => _client.PatchAsync(account, $"{RulesPath}/{Uri.EscapeDataString(rule.Id)}", body, ct)); + rule.Sequence = target; // keep the local model in sync with what the server was told + patched++; } - LogService.Log($"ServerRules: reordered {ruleIdsInOrder.Count} rule(s) for {account.Username}"); + LogService.Log($"ServerRules: reordered {rulesInOrder.Count} rule(s), {patched} PATCHed, for {account.Username}"); } public async Task DeleteAsync(Guid accountId, string ruleId, CancellationToken ct = default) diff --git a/QuickMail/Services/IServerRuleService.cs b/QuickMail/Services/IServerRuleService.cs index 92f68621..a4907120 100644 --- a/QuickMail/Services/IServerRuleService.cs +++ b/QuickMail/Services/IServerRuleService.cs @@ -38,8 +38,12 @@ public interface IServerRuleService /// Enables or disables a rule. Safe even for rules outside the editable subset. Task SetEnabledAsync(Guid accountId, string ruleId, bool enabled, CancellationToken ct = default); - /// Rewrites execution order; the given ids are assigned sequences 1..n in order. - Task ReorderAsync(Guid accountId, IReadOnlyList ruleIdsInOrder, CancellationToken ct = default); + /// + /// Applies the given rule order by reassigning the existing server sequence values in that order, + /// PATCHing only the rules whose sequence actually changes (so a server-protected rule elsewhere + /// in the list is never touched). Each model carries its current . + /// + Task ReorderAsync(Guid accountId, IReadOnlyList rulesInOrder, CancellationToken ct = default); Task DeleteAsync(Guid accountId, string ruleId, CancellationToken ct = default); } diff --git a/QuickMail/ViewModels/ServerRuleEditorViewModel.cs b/QuickMail/ViewModels/ServerRuleEditorViewModel.cs index cbfea38c..bfd74366 100644 --- a/QuickMail/ViewModels/ServerRuleEditorViewModel.cs +++ b/QuickMail/ViewModels/ServerRuleEditorViewModel.cs @@ -32,8 +32,12 @@ public partial class ServerRuleEditorViewModel : ObservableObject /// Ask the View to open the folder picker; returns the chosen folder id (or null). public event Func<(string Id, string Name)?>? PickFolderRequested; - /// Raised on a successful Save with the assembled rule. The owner persists it. - public event Action? Saved; + /// + /// Raised on Save with the assembled rule; the owner persists it and returns an error message on + /// failure (null on success). The editor stays open and shows the error when non-null, so a + /// rejected save never silently loses the form. + /// + public event Func>? Saved; /// Raised when the editor window should close (Save or Cancel). public event Action? CloseRequested; @@ -83,6 +87,9 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) string.Equals(o.Value, rule.Importance, StringComparison.OrdinalIgnoreCase)) ?? ImportanceOptions[0]; vm.SelectedMarkImportance = ImportanceOptions.FirstOrDefault(o => string.Equals(o.Value, rule.MarkImportance, StringComparison.OrdinalIgnoreCase)) ?? ImportanceOptions[0]; + // If the rule already uses any advanced field, open the Advanced section so editing never + // hides a populated field. A brand-new rule leaves it collapsed. + vm.IsAdvancedExpanded = vm.HasAdvancedContent(); return vm; } @@ -91,6 +98,12 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) [ObservableProperty] private string _name = string.Empty; [ObservableProperty] private bool _isEnabled = true; + /// + /// Whether the Advanced conditions/actions section is expanded. Collapsed for a new rule; opened + /// automatically when editing a rule that already uses an advanced field (see ). + /// + [ObservableProperty] private bool _isAdvancedExpanded; + // Conditions [ObservableProperty] private string _senderContains = string.Empty; [ObservableProperty] private string _fromAddresses = string.Empty; @@ -127,6 +140,8 @@ public static ServerRuleEditorViewModel ForEdit(ServerRuleModel rule) [ObservableProperty] private string _nameError = string.Empty; [ObservableProperty] private string _folderError = string.Empty; [ObservableProperty] private string _actionsError = string.Empty; + /// A server-side save failure (e.g. Graph rejected the rule), shown on the form. + [ObservableProperty] private string _saveError = string.Empty; /// Importance choices for both the condition and the action ComboBoxes. public static List ImportanceOptions { get; } = @@ -160,11 +175,22 @@ private void PickCopyFolder() } [RelayCommand] - private void Save() + private async Task Save() { if (!Validate()) return; - Saved?.Invoke(ToModel()); - CloseRequested?.Invoke(); + SaveError = string.Empty; + + // The owner persists and returns null on success, or an error to display. Close only on + // success — a failed save keeps the form (and the user's input) and shows why. + var error = Saved is null ? null : await Saved.Invoke(ToModel()); + if (string.IsNullOrEmpty(error)) + { + CloseRequested?.Invoke(); + return; + } + + SaveError = error; + AnnouncementRequested?.Invoke(error, AnnouncementCategory.Result); } [RelayCommand] @@ -246,6 +272,21 @@ public bool Validate() return valid; } + /// + /// True when any field that lives in the Advanced section is set — used to auto-expand it when + /// editing. Keep this list in sync with the Advanced group in ServerRuleEditorWindow.xaml. + /// + private bool HasAdvancedContent() + => !string.IsNullOrWhiteSpace(SenderContains) + || !string.IsNullOrWhiteSpace(SentToAddresses) + || !string.IsNullOrWhiteSpace(BodyOrSubjectContains) + || !string.IsNullOrWhiteSpace(BodyContains) + || SentToMe || SentOnlyToMe || HasAttachments + || !string.IsNullOrWhiteSpace(SelectedImportance?.Value) + || CopyToFolder + || !string.IsNullOrWhiteSpace(SelectedMarkImportance?.Value) + || !string.IsNullOrWhiteSpace(ForwardTo); + private bool HasAnyAction() => (MoveToFolder && !string.IsNullOrWhiteSpace(MoveToFolderId)) || (CopyToFolder && !string.IsNullOrWhiteSpace(CopyToFolderId)) diff --git a/QuickMail/ViewModels/ServerRulesViewModel.cs b/QuickMail/ViewModels/ServerRulesViewModel.cs index a0cffbf5..1502908b 100644 --- a/QuickMail/ViewModels/ServerRulesViewModel.cs +++ b/QuickMail/ViewModels/ServerRulesViewModel.cs @@ -39,6 +39,13 @@ public partial class ServerRulesViewModel : ObservableObject /// The View should open the rule editor (modeless) for this prepared editor VM. public event Action? EditorRequested; + /// + /// After an action that changed a rule in place (toggle, move, delete), the View should return + /// keyboard focus to the selected rule in the list — so the user isn't stranded on a button, and + /// (for toggle) the screen reader re-announces the updated row when focus lands back on it. + /// + public event Action? FocusSelectedRuleRequested; + // ── Construction ──────────────────────────────────────────────────────── private readonly IReadOnlyDictionary>? _foldersByAccount; @@ -46,7 +53,8 @@ public partial class ServerRulesViewModel : ObservableObject public ServerRulesViewModel( IServerRuleService service, IEnumerable graphAccounts, - IReadOnlyDictionary>? foldersByAccount = null) + IReadOnlyDictionary>? foldersByAccount = null, + Guid? preferredAccountId = null) { _service = service; _foldersByAccount = foldersByAccount; @@ -56,7 +64,12 @@ public ServerRulesViewModel( .Select(a => new AccountOption { Id = a.Id, DisplayName = a.AccountLabel }) .ToList(); - _selectedAccount = AccountOptions.FirstOrDefault(); + // Land on the account the user is currently in (the inbox they opened the Rules Manager from) + // rather than always the first Graph account — otherwise opening from the Guest inbox would + // show icanbrew's rules. Falls back to the first account when there's no current-account + // context (e.g. an aggregate/unified view at the top of the tree). + _selectedAccount = AccountOptions.FirstOrDefault(o => o.Id == preferredAccountId) + ?? AccountOptions.FirstOrDefault(); } /// @@ -93,8 +106,24 @@ private void ResolveFolderNames(ServerRuleModel rule) [NotifyPropertyChangedFor(nameof(CanEditSelected))] [NotifyPropertyChangedFor(nameof(CanModifySelected))] [NotifyPropertyChangedFor(nameof(DetailText))] + [NotifyPropertyChangedFor(nameof(ToggleEnabledLabel))] + [NotifyCanExecuteChangedFor(nameof(MoveUpCommand))] + [NotifyCanExecuteChangedFor(nameof(MoveDownCommand))] + [NotifyCanExecuteChangedFor(nameof(EditRuleCommand))] + [NotifyCanExecuteChangedFor(nameof(ToggleEnabledCommand))] + [NotifyCanExecuteChangedFor(nameof(DeleteRuleCommand))] private ServerRuleModel? _selectedRule; + /// Enable/Disable button text: "Enable" for a disabled rule, "Disable" for an enabled one. + public string ToggleEnabledLabel => SelectedRule?.IsEnabled == true ? "Disable" : "Enable"; + + /// Move Up is invalid for the first rule, a server read-only rule, or none selected. + /// A read-only rule can't be re-sequenced (Graph refuses the PATCH), so gate it like Edit/Delete. + public bool CanMoveUp => SelectedRule is { IsReadOnly: false } r && Rules.IndexOf(r) > 0; + + /// Move Down is invalid for the last rule, a server read-only rule, or none selected. + public bool CanMoveDown => SelectedRule is { IsReadOnly: false } r && Rules.IndexOf(r) is var i && i >= 0 && i < Rules.Count - 1; + [ObservableProperty] private AccountOption? _selectedAccount; [ObservableProperty] private string _statusText = string.Empty; [ObservableProperty] private bool _isBusy; @@ -104,16 +133,20 @@ private void ResolveFolderNames(ServerRuleModel rule) /// represent — saving those would replace the server's richer object with our narrower one and /// silently drop the user's other predicates (spec §16). /// - /// Deliberately NOT wired to the commands' CanExecute. A disabled control is - /// announced only as unavailable, which tells a screen-reader user nothing about why a - /// rule on their own mailbox can't be edited. Instead the commands stay executable and explain - /// the reason when invoked (see ), which is the accessible behaviour. XAML - /// may still bind visual affordances to this property. + /// Wired to EditRuleCommand.CanExecute so the Edit button and context-menu item are + /// disabled for these rules. (Tim's call, 2026-07-25, reversing the earlier + /// keep-enabled-and-explain design: he runs with announcements off, so an explain-on-invoke is + /// inaudible — a pressable item that silently fails is worse than a disabled one. The list row + /// already states "read-only" / "not editable in QuickMail", so the reason is still conveyed.) /// /// public bool CanEditSelected => SelectedRule is { IsFullyEditable: true, IsReadOnly: false }; - /// Toggle/reorder/delete only need the rule to be writable, not fully representable. + /// + /// Toggle/reorder/delete only need the rule to be writable, not fully representable. Wired to + /// those commands' CanExecute so they're disabled for read-only rules (see + /// for the rationale). + /// public bool CanModifySelected => SelectedRule is { IsReadOnly: false }; /// Full prose for the detail region, including parts QuickMail can't edit. @@ -176,61 +209,53 @@ private void CreateRule() if (SelectedAccount?.Id is not Guid accountId) return; var editor = ServerRuleEditorViewModel.ForNew(); - editor.Saved += async rule => await SaveNewAsync(accountId, rule); + editor.Saved += rule => SaveNewAsync(accountId, rule); EditorRequested?.Invoke(editor); } - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanEditSelected))] private void EditRule() { if (SelectedAccount?.Id is not Guid accountId) return; if (SelectedRule is not { } rule) return; - - if (!CanEditSelected) - { - var why = rule.IsReadOnly - ? "This rule is read-only on the server and can't be changed here." - : "This rule uses conditions or actions QuickMail can't edit yet. You can enable, disable, or delete it here, or edit it in Outlook."; - StatusText = why; - Announce(why, AnnouncementCategory.Hint); - return; - } + if (!CanEditSelected) return; // defensive; the command is disabled for these rules var editor = ServerRuleEditorViewModel.ForEdit(rule); - editor.Saved += async updated => await SaveExistingAsync(accountId, updated); + editor.Saved += updated => SaveExistingAsync(accountId, updated); EditorRequested?.Invoke(editor); } - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanModifySelected))] private async Task ToggleEnabledAsync(CancellationToken ct) { if (SelectedAccount?.Id is not Guid accountId) return; if (SelectedRule is not { } rule) return; - if (!CanModifySelected) return; + if (!CanModifySelected) return; // defensive; the command is disabled for read-only rules var target = !rule.IsEnabled; await RunWriteAsync(async () => { await _service.SetEnabledAsync(accountId, rule.Id, target, ct); - rule.IsEnabled = target; - RefreshRow(rule); + rule.IsEnabled = target; // observable → the row's RowText updates in place OnPropertyChanged(nameof(DetailText)); + OnPropertyChanged(nameof(ToggleEnabledLabel)); Announce(target ? "Rule enabled." : "Rule disabled.", AnnouncementCategory.Result); + // Focus returns to the row (so the new state is read) via RunWriteAsync's finally. }); } - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanMoveUp))] private Task MoveUpAsync(CancellationToken ct) => MoveAsync(-1, ct); - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanMoveDown))] private Task MoveDownAsync(CancellationToken ct) => MoveAsync(+1, ct); - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanModifySelected))] private async Task DeleteRuleAsync(CancellationToken ct) { if (SelectedAccount?.Id is not Guid accountId) return; if (SelectedRule is not { } rule) return; - if (!CanModifySelected) return; + if (!CanModifySelected) return; // defensive; the command is disabled for read-only rules var confirmed = ConfirmDeleteRequested?.Invoke( $"Delete server rule '{rule.DisplayName}'? It will stop running on the server.", @@ -247,34 +272,12 @@ await RunWriteAsync(async () => SelectedRule = Rules.Count == 0 ? null : Rules[Math.Min(index, Rules.Count - 1)]; Announce("Rule deleted.", AnnouncementCategory.Result); + // Focus moves to the newly-selected neighbour via RunWriteAsync's finally. }); } // ── Internals ─────────────────────────────────────────────────────────── - /// - /// Forces the list row to re-render after the rule was mutated in place. - /// - /// A row's displayed and announced text comes from , - /// which a ListView evaluates when it realises the container. - /// is a plain model (like ) and raises no change notification, so mutating - /// IsEnabled alone would leave the row still reading "enabled" after the user disabled it. - /// Re-assigning the slot raises a Replace notification, which regenerates that one container. - /// - /// - /// Chosen over making the model observable because ToString() composes several properties; - /// per-property notification would not cause WPF to re-evaluate it anyway. - /// - /// - private void RefreshRow(ServerRuleModel rule) - { - var index = Rules.IndexOf(rule); - if (index < 0) return; - - Rules[index] = rule; - SelectedRule = rule; // Replace clears selection; put it back so focus doesn't jump - } - private async Task MoveAsync(int delta, CancellationToken ct) { if (SelectedAccount?.Id is not Guid accountId) return; @@ -289,30 +292,46 @@ private async Task MoveAsync(int delta, CancellationToken ct) await RunWriteAsync(async () => { - await _service.ReorderAsync(accountId, Rules.Select(r => r.Id).ToList(), ct); - - // Keep local sequence numbers consistent with what the server was just told. - for (var i = 0; i < Rules.Count; i++) Rules[i].Sequence = i + 1; + // Pass the models (each carries its current server Sequence). The service reassigns only + // the changed sequence values, so a server-protected rule elsewhere isn't PATCHed and + // can't 400 this move. + await _service.ReorderAsync(accountId, Rules.ToList(), ct); Announce($"Moved {(delta < 0 ? "up" : "down")}. Now {to + 1} of {Rules.Count}.", AnnouncementCategory.Status); + // The rule's position changed → Move Up/Down availability may have flipped (now at an end). + MoveUpCommand.NotifyCanExecuteChanged(); + MoveDownCommand.NotifyCanExecuteChanged(); + OnPropertyChanged(nameof(CanMoveUp)); + OnPropertyChanged(nameof(CanMoveDown)); + // Focus follows the rule to its new position via RunWriteAsync's finally. }, onFailure: () => Rules.Move(to, from)); // put it back if the server refused } - private async Task SaveNewAsync(Guid accountId, ServerRuleModel rule) + /// Persists a new rule. Returns null on success, or an error message for the editor. + private async Task SaveNewAsync(Guid accountId, ServerRuleModel rule) { - await RunWriteAsync(async () => + // Graph rejects sequence 0 (must be 1-based); a new rule goes to the end of the list. + rule.Sequence = Rules.Count + 1; + + // focusSelectedAfter:false — the editor is still open during the write; don't pull focus to + // the list. On success we focus the new rule *after* the editor closes (below). + var error = await RunWriteAsync(async () => { var created = await _service.CreateAsync(accountId, rule); Rules.Add(created); SelectedRule = created; Announce("Rule created.", AnnouncementCategory.Result); - }); + }, focusSelectedAfter: false); + + if (error is null) FocusSelectedRuleRequested?.Invoke(); // land on the new rule once the editor closes + return error; } - private async Task SaveExistingAsync(Guid accountId, ServerRuleModel rule) + /// Persists an edited rule. Returns null on success, or an error message for the editor. + private async Task SaveExistingAsync(Guid accountId, ServerRuleModel rule) { - await RunWriteAsync(async () => + var error = await RunWriteAsync(async () => { await _service.UpdateAsync(accountId, rule); @@ -321,35 +340,55 @@ await RunWriteAsync(async () => SelectedRule = rule; Announce("Rule updated.", AnnouncementCategory.Result); - }); + }, focusSelectedAfter: false); + + if (error is null) FocusSelectedRuleRequested?.Invoke(); + return error; } /// /// Runs a write, translating a permission refusal into the admin-directed path and never - /// swallowing other failures into a silent no-op. + /// swallowing other failures into a silent no-op. Returns null on success, or a user-facing + /// error message (which the editor shows when the write came from a Save). /// - private async Task RunWriteAsync(Func write, Action? onFailure = null) + /// + /// When true (the in-list actions: toggle/move/delete), focus returns to the selected rule so the + /// user isn't stranded on a button. Save paths pass false — the editor is still open during the + /// write and manages its own focus (staying on the error, or focusing the list after it closes). + /// + private async Task RunWriteAsync(Func write, Action? onFailure = null, bool focusSelectedAfter = true) { IsBusy = true; try { await write(); + return null; } catch (ServerRuleConsentRequiredException ex) { onFailure?.Invoke(); HandlePermissionRefusal(ex); + return StatusText; } catch (Exception ex) { onFailure?.Invoke(); - StatusText = ex.Message; + // Some rules (typically created by Outlook) can't be modified through the Graph API at + // all — the server returns ErrorNotSupportedMessageRule. Translate that into a plain, + // actionable message instead of surfacing raw HTTP/JSON. + StatusText = ex.Message.Contains("ErrorNotSupportedMessageRule", StringComparison.OrdinalIgnoreCase) + ? $"'{SelectedRule?.DisplayName}' can't be changed from QuickMail — edit it in Outlook." + : ex.Message; Announce(StatusText, AnnouncementCategory.Result); LogService.Log("ServerRules: write failed", ex); + return StatusText; } finally { IsBusy = false; + // Put keyboard focus back on the selected rule so the user isn't stranded on a button + // (and, after a failed move, so arrow keys stay in the list). Suppressed for Save paths. + if (focusSelectedAfter && SelectedRule is not null) FocusSelectedRuleRequested?.Invoke(); } } diff --git a/QuickMail/Views/MainWindow.xaml.cs b/QuickMail/Views/MainWindow.xaml.cs index 356f8490..ed2332bf 100644 --- a/QuickMail/Views/MainWindow.xaml.cs +++ b/QuickMail/Views/MainWindow.xaml.cs @@ -3431,7 +3431,13 @@ private void OnActivated(object? sender, EventArgs e) { LogService.Debug($"[FOCUS] Activated lastPane={_paneIndexBeforeDeactivation} {FocusInfo()}"); if (_paneIndexBeforeDeactivation == 3 || _paneIndexBeforeDeactivation == 4) - Dispatcher.InvokeAsync(ReturnFocusToMessageList, DispatcherPriority.Input); + // Re-check IsActive at callback time: a transient activation (e.g. a modeless child of the + // Rules Manager closing and briefly bouncing foreground through here) must NOT pull focus + // into this window's message list when another window ends up active. Otherwise this + // unconditionally steals focus from the Rules Manager's rule list. (#333) + Dispatcher.InvokeAsync( + () => { if (IsActive) ReturnFocusToMessageList(); }, + DispatcherPriority.Input); } // Routes focus to whichever message panel is currently visible. @@ -5855,7 +5861,11 @@ private void OpenRulesManager(MailRule? template = null) if (_serverRuleService != null && _featureGate.IsEnabled(FeatureFlag.ServerRules) && accounts.Any(a => a.BackendKind == BackendKind.MicrosoftGraph)) - serverRulesVm = new ServerRulesViewModel(_serverRuleService, accounts, _vm.CachedFolders); + // Seed the picker with the account the user is currently in, so the Rules Manager opens + // on that account's rules rather than always the first Graph account (#333). Null on + // aggregate views → VM falls back to the first account. + serverRulesVm = new ServerRulesViewModel( + _serverRuleService, accounts, _vm.CachedFolders, _vm.SelectedAccount?.Id); var dialog = new RulesManagerWindow(rulesVm, accounts, _vm.CachedFolders, serverRulesVm); _rulesWindow = dialog; diff --git a/QuickMail/Views/RulesManagerWindow.xaml b/QuickMail/Views/RulesManagerWindow.xaml index 78f93c45..e9b96bd7 100644 --- a/QuickMail/Views/RulesManagerWindow.xaml +++ b/QuickMail/Views/RulesManagerWindow.xaml @@ -286,14 +286,43 @@ Margin="0,10,0,0" Padding="8" BorderThickness="1" BorderBrush="{DynamicResource Theme.Border}"> - + + + + + + + + + +