diff --git a/QuickMail.Tests/GraphServerRuleServiceTests.cs b/QuickMail.Tests/GraphServerRuleServiceTests.cs index 8501f6e2..dae19511 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) { @@ -370,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] @@ -444,6 +506,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..1bae281a 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] @@ -316,6 +412,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() { @@ -361,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 "; @@ -371,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 @@ -385,8 +511,27 @@ public void Editor_Save_RaisesSavedWithAssembledRule_AndCloses() } [Fact] - public void Editor_ForEdit_RoundTripsFields() + 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() { + // 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", @@ -395,28 +540,209 @@ 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); } + // ── Classification: server vs client (spec §20.3) ─────────────────────── + + [Fact] + public void Classify_SimpleRuleOnGraphAccount_IsServerRule() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Move digests"; + e.SubjectContains = "digest"; + e.MoveToFolder = true; e.MoveToFolderId = "f1"; + + var result = e.Classify(accountSupportsServerRules: true); + + Assert.Equal(RuleRunsWhere.Server, result.Kind); + Assert.False(result.IsConflict); + } + + [Fact] + public void Classify_MarkAsUnreadOnGraphAccount_IsClientRule_WithReason() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Keep unread"; + e.SubjectContains = "later"; + e.MarkAsUnread = true; // the only client-only action today + + var result = e.Classify(accountSupportsServerRules: true); + + Assert.Equal(RuleRunsWhere.Client, result.Kind); + Assert.Contains("Mark as unread", result.ClientReason); + } + + [Fact] + public void Classify_SimpleRuleOnNonGraphAccount_IsClientRule() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "IMAP rule"; + e.SubjectContains = "news"; + e.MarkAsRead = true; + + var result = e.Classify(accountSupportsServerRules: false); + + Assert.Equal(RuleRunsWhere.Client, result.Kind); + Assert.Contains("server-side", result.ClientReason); + } + + [Fact] + public void Classify_ClientOnlyActionPlusServerOnlyCondition_IsConflict() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Impossible"; + e.MarkAsUnread = true; // client-only action + e.SelectedImportance = ServerRuleEditorViewModel.ImportanceOptions.First(o => o.Value == "high"); // server-only condition + + var result = e.Classify(accountSupportsServerRules: true); + + Assert.Null(result.Kind); + Assert.True(result.IsConflict); + Assert.Contains("Mark as unread", result.ConflictError); + Assert.Contains("importance", result.ConflictError); + } + + [Fact] + public void Classify_ServerOnlyActionOnNonGraphAccount_IsConflict() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Copy on IMAP"; + e.CopyToFolder = true; e.CopyToFolderId = "f2"; // server-only action, no client equivalent + + var result = e.Classify(accountSupportsServerRules: false); + + Assert.True(result.IsConflict); + Assert.Contains("Copy to folder", result.ConflictError); + } + + [Fact] + public void Classify_MultipleActionsWithoutClientOnly_StaysServer() + { + // Several actions is fine for a server rule; only a client-only capability forces client. + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Multi"; + e.SubjectContains = "x"; + e.MarkAsRead = true; + e.MoveToFolder = true; e.MoveToFolderId = "f1"; + + Assert.Equal(RuleRunsWhere.Server, e.Classify(accountSupportsServerRules: true).Kind); + } + + // ── Client-rule conversion (spec §20.4) ───────────────────────────────── + + [Fact] + public void ToClientRule_MapsConditionsAndMoveAction() + { + var accountId = Guid.NewGuid(); + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = " From Bob "; + e.SenderContains = "bob@x.com"; + e.SubjectContains = "report"; + e.HasAttachments = true; + e.MoveToFolder = true; e.MoveToFolderId = "Inbox/Reports"; + + var rule = e.ToClientRule(accountId); + + Assert.Equal("From Bob", rule.Name); // trimmed + Assert.Equal(accountId, rule.AccountId); + Assert.True(rule.UseFromCondition); + Assert.Equal("bob@x.com", rule.FromContains); + Assert.True(rule.UseSubjectCondition); + Assert.Equal("report", rule.SubjectContains); + Assert.False(rule.UseToCondition); + Assert.True(rule.MustHaveAttachments); + Assert.Equal(RuleAction.MoveToFolder, rule.Action); + Assert.Equal("Inbox/Reports", rule.TargetFolder); + } + + [Theory] + [InlineData(true, false, false, RuleAction.MarkAsUnread)] + [InlineData(false, true, false, RuleAction.Delete)] + [InlineData(false, false, true, RuleAction.MarkAsRead)] + public void ToClientRule_MapsTheSingleAction(bool unread, bool delete, bool read, RuleAction expected) + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "Act"; + e.SubjectContains = "x"; + e.MarkAsUnread = unread; e.Delete = delete; e.MarkAsRead = read; + + Assert.Equal(expected, e.ToClientRule(Guid.NewGuid()).Action); + } + + [Fact] + public void ToClientRule_SingleFromAddress_BecomesFromContains() + { + var e = ServerRuleEditorViewModel.ForNew(); + e.Name = "One sender"; + e.FromAddresses = "alice@x.com"; + e.MarkAsRead = true; + + var rule = e.ToClientRule(Guid.NewGuid()); + Assert.True(rule.UseFromCondition); + Assert.Equal("alice@x.com", rule.FromContains); + } + + [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() + { + 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.Tests/UnifiedRuleRowTests.cs b/QuickMail.Tests/UnifiedRuleRowTests.cs new file mode 100644 index 00000000..620f0ae6 --- /dev/null +++ b/QuickMail.Tests/UnifiedRuleRowTests.cs @@ -0,0 +1,63 @@ +using QuickMail.Models; +using Xunit; + +namespace QuickMail.Tests; + +public class UnifiedRuleRowTests +{ + [Fact] + public void ServerRow_ReadsFromServerRule_AndMarksOnServer() + { + var row = UnifiedRuleRow.ForServer(new ServerRuleModel + { + DisplayName = "Move digests", + IsEnabled = true, + SubjectContains = "digest", + MoveToFolderId = "f1", + MoveToFolderName = "Archive", + }); + + Assert.Equal(RuleRunsWhere.Server, row.RunsWhere); + Assert.Equal("Move digests", row.Name); + Assert.True(row.IsEnabled); + Assert.Contains("on server", row.RowText); + Assert.Contains("enabled", row.RowText); + Assert.Contains("Archive", row.RowText); // from the server summary + Assert.Equal(row.RowText, row.ToString()); // screen reader reads ToString() + } + + [Fact] + public void ClientRow_ReadsFromMailRule_AndMarksInQuickMail() + { + var row = UnifiedRuleRow.ForClient(new MailRule + { + Name = "Keep unread", + IsEnabled = false, + UseSubjectCondition = true, + SubjectContains = "later", + Action = RuleAction.MarkAsUnread, + }); + + Assert.Equal(RuleRunsWhere.Client, row.RunsWhere); + Assert.Equal("Keep unread", row.Name); + Assert.False(row.IsEnabled); + Assert.Contains("in QuickMail", row.RowText); + Assert.Contains("disabled", row.RowText); + Assert.Contains("subject contains 'later'", row.RowText); + Assert.Contains("mark as unread", row.RowText); + } + + [Fact] + public void ClientRow_NoConditions_ReadsAllMessages() + { + var row = UnifiedRuleRow.ForClient(new MailRule + { + Name = "Catch-all", + Action = RuleAction.MoveToFolder, + TargetFolder = "INBOX/Sorted", + }); + + Assert.Contains("All messages", row.RowText); + Assert.Contains("move to INBOX/Sorted", row.RowText); + } +} 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/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/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/Models/ServerRuleModel.cs b/QuickMail/Models/ServerRuleModel.cs index 4790a45e..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; } @@ -38,8 +50,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 +75,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 +125,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 +154,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 +218,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/Models/UnifiedRuleRow.cs b/QuickMail/Models/UnifiedRuleRow.cs new file mode 100644 index 00000000..7f5d0cca --- /dev/null +++ b/QuickMail/Models/UnifiedRuleRow.cs @@ -0,0 +1,82 @@ +using System.Collections.Generic; +using System.Linq; + +namespace QuickMail.Models; + +/// Where a rule executes: on the Microsoft 365 server, or inside QuickMail. +public enum RuleRunsWhere { Server, Client } + +/// +/// One row in the unified per-account rules list (spec §20.7). Wraps exactly one of a server +/// () or client () rule and presents a single, +/// consistent accessible line that also states where the rule runs. The list is per-account, so the +/// row carries no account label. +/// +public sealed class UnifiedRuleRow +{ + private UnifiedRuleRow(RuleRunsWhere runsWhere, ServerRuleModel? server, MailRule? client) + { + RunsWhere = runsWhere; + Server = server; + Client = client; + } + + public static UnifiedRuleRow ForServer(ServerRuleModel rule) => new(RuleRunsWhere.Server, rule, null); + public static UnifiedRuleRow ForClient(MailRule rule) => new(RuleRunsWhere.Client, null, rule); + + public RuleRunsWhere RunsWhere { get; } + + /// The wrapped server rule, or null for a client row. + public ServerRuleModel? Server { get; } + + /// The wrapped client rule, or null for a server row. + public MailRule? Client { get; } + + public string Name => RunsWhere == RuleRunsWhere.Server ? Server!.DisplayName : Client!.Name; + + public bool IsEnabled => RunsWhere == RuleRunsWhere.Server ? Server!.IsEnabled : Client!.IsEnabled; + + /// + /// The list row's accessible/display text: name, where it runs, enabled state, and a one-line + /// summary. A screen reader reads a Selector item's name from , so that + /// forwards here (see CLAUDE.md). + /// + public string RowText + { + get + { + var name = string.IsNullOrWhiteSpace(Name) ? "Unnamed rule" : Name; + var where = RunsWhere == RuleRunsWhere.Server ? "on server" : "in QuickMail"; + var state = IsEnabled ? "enabled" : "disabled"; + var head = $"{name}, {where}, {state}"; + var summary = RunsWhere == RuleRunsWhere.Server ? Server!.OneLineSummary() : ClientSummary(Client!); + return string.IsNullOrEmpty(summary) ? head : $"{head}. {summary}"; + } + } + + public override string ToString() => RowText; + + /// "If subject contains 'x' → move to Archive" for a client rule — mirrors + /// so both kinds read the same way. + private static string ClientSummary(MailRule r) + { + var conditions = new List(); + if (r.UseFromCondition && !string.IsNullOrWhiteSpace(r.FromContains)) conditions.Add($"from contains '{r.FromContains}'"); + if (r.UseToCondition && !string.IsNullOrWhiteSpace(r.ToContains)) conditions.Add($"to contains '{r.ToContains}'"); + if (r.UseSubjectCondition && !string.IsNullOrWhiteSpace(r.SubjectContains)) conditions.Add($"subject contains '{r.SubjectContains}'"); + if (r.UseBodyCondition && !string.IsNullOrWhiteSpace(r.BodyContains)) conditions.Add($"body contains '{r.BodyContains}'"); + if (r.MustHaveAttachments) conditions.Add("has attachments"); + + var action = r.Action switch + { + RuleAction.MarkAsRead => "mark as read", + RuleAction.MarkAsUnread => "mark as unread", + RuleAction.MoveToFolder => $"move to {(string.IsNullOrWhiteSpace(r.TargetFolder) ? "a folder" : r.TargetFolder)}", + RuleAction.Delete => "move to Trash", + _ => r.Action.ToString(), + }; + + var lhs = conditions.Count == 0 ? "All messages" : "If " + string.Join(" and ", conditions); + return $"{lhs} → {action}"; + } +} 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/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..961d7c9a 100644 --- a/QuickMail/Services/GraphServerRuleService.cs +++ b/QuickMail/Services/GraphServerRuleService.cs @@ -50,6 +50,19 @@ 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)}]"); + 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; } @@ -96,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 7a4f562e..6eda92cf 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; @@ -59,8 +63,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 +74,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), @@ -78,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; } @@ -86,11 +98,19 @@ 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; + [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,18 +122,28 @@ 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; + /// Client-only action — Microsoft 365 server rules have no "mark as unread" (spec §20.2). + [ObservableProperty] private bool _markAsUnread; [ObservableProperty] private ImportanceOption _selectedMarkImportance = ImportanceOptions[0]; [ObservableProperty] private bool _delete; [ObservableProperty] private string _forwardTo = string.Empty; [ObservableProperty] private bool _stopProcessingRules; public bool IsMoveToFolderSelected => MoveToFolder; + public bool IsCopyToFolderSelected => CopyToFolder; // Validation surfaces [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; } = @@ -137,11 +167,32 @@ private void PickFolder() } [RelayCommand] - private void Save() + private void PickCopyFolder() + { + if (PickFolderRequested?.Invoke() is not { } picked) return; + CopyToFolderId = picked.Id; + CopyToFolderName = picked.Name; + CopyToFolder = true; + FolderError = string.Empty; + } + + [RelayCommand] + 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] @@ -158,8 +209,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 +220,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, @@ -181,6 +236,49 @@ private void Save() RawExceptions = _rawExceptions, }; + /// + /// Assembles a client-side from the client-representable subset of the + /// form (spec §20.4). Only valid when holds — the caller + /// guarantees a single From/To value and exactly one action, so the mapping is lossless. The + /// client engine treats a condition as active only when its flag is set AND it has a value, so + /// empty conditions are simply switched off. + /// + public MailRule ToClientRule(Guid accountId) + { + var fromAddrs = SplitAddresses(FromAddresses); + var from = !string.IsNullOrWhiteSpace(SenderContains) ? SenderContains.Trim() + : fromAddrs.Count == 1 ? fromAddrs[0] + : null; + var to = SplitAddresses(SentToAddresses) is { Count: 1 } toList ? toList[0] : null; + var subject = Blank(SubjectContains); + var body = Blank(BodyContains); + + return new MailRule + { + Name = Name.Trim(), + IsEnabled = IsEnabled, + AccountId = accountId, + + UseFromCondition = from is not null, FromContains = from, + UseToCondition = to is not null, ToContains = to, + UseSubjectCondition = subject is not null, SubjectContains = subject, + UseBodyCondition = body is not null, BodyContains = body, + MustHaveAttachments = HasAttachments, + + Action = ClientAction(), + TargetFolder = MoveToFolder ? MoveToFolderId : null, + }; + } + + /// The single client action in use (IsClientRepresentable guarantees exactly one). + private RuleAction ClientAction() + { + if (MoveToFolder) return RuleAction.MoveToFolder; + if (Delete) return RuleAction.Delete; + if (MarkAsUnread) return RuleAction.MarkAsUnread; + return RuleAction.MarkAsRead; + } + public bool Validate() { NameError = FolderError = ActionsError = string.Empty; @@ -198,6 +296,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."; @@ -213,9 +317,124 @@ public bool Validate() return valid; } + // ── Classification: server vs client (spec §20.3) ─────────────────────── + + /// + /// Decides where the rule runs. A Graph account gets a server rule unless the rule uses a + /// client-only capability; otherwise (or on a non-Graph account) it's a client rule, with a + /// reason for the save dialog. A rule that fits neither — a client-only action combined with a + /// server-only condition/action — is a conflict the user must resolve. Assumes the rule already + /// passed (so it has at least one action). + /// + public RuleClassification Classify(bool accountSupportsServerRules) + { + if (accountSupportsServerRules && IsServerRepresentable) + return new RuleClassification { Kind = RuleRunsWhere.Server }; + + if (IsClientRepresentable) + { + var reason = accountSupportsServerRules + ? $"it uses {Join(ClientOnlyFeaturesUsed())}, which Microsoft 365 server rules don't support" + : "this account doesn't support server-side rules"; + return new RuleClassification { Kind = RuleRunsWhere.Client, ClientReason = reason }; + } + + // Representable by neither: a client-only action combined with a server-only condition/action, + // or a server-only feature on a non-Graph account. + var serverOnly = ServerOnlyFeaturesUsed(); + var clientOnly = ClientOnlyFeaturesUsed(); + var conflict = accountSupportsServerRules && clientOnly.Count > 0 + ? $"{Join(clientOnly)} only works in a QuickMail rule, but {Join(serverOnly)} only works in a server rule. Remove one to save." + : $"This account only supports QuickMail rules, but {Join(serverOnly)} isn't available in a QuickMail rule. Remove it to save."; + return new RuleClassification { ConflictError = conflict }; + } + + /// True when the rule uses no client-only capability, so the server can express it. + public bool IsServerRepresentable => ClientOnlyFeaturesUsed().Count == 0; + + /// + /// True when every condition and action fits the client rule model (a near-subset of the server + /// model): no server-only condition/action, single From/To value, exactly one action. + /// + public bool IsClientRepresentable + => ServerOnlyFeaturesUsed().Count == 0 && ClientEligibleActionCount() == 1; + + /// Client-only capabilities in use — the server has no equivalent (spec §20.2). Extend + /// as more client-only options are added (play sound, notify, …). + private List ClientOnlyFeaturesUsed() + { + var f = new List(); + if (MarkAsUnread) f.Add("Mark as unread"); + return f; + } + + /// + /// Features only a server rule can express, so any of them blocks representing the rule as a + /// client rule: conditions with no client equivalent, the client's single-value From/To limits, + /// server-only actions, and the client's one-action limit. + /// + private List ServerOnlyFeaturesUsed() + { + var f = new List(); + + // Conditions with no client equivalent. + if (!string.IsNullOrWhiteSpace(BodyOrSubjectContains)) f.Add("the subject-or-body condition"); + if (SentToMe) f.Add("the “sent to me” condition"); + if (SentOnlyToMe) f.Add("the “sent only to me” condition"); + if (SelectedImportance?.Value is not null) f.Add("the importance condition"); + + // A client rule has a single From and a single To field. + var fromAddrs = SplitAddresses(FromAddresses); + if (fromAddrs.Count > 1) f.Add("multiple From addresses"); + if (!string.IsNullOrWhiteSpace(SenderContains) && fromAddrs.Count > 0) + f.Add("both Sender-contains and From-addresses"); + if (SplitAddresses(SentToAddresses).Count > 1) f.Add("multiple Sent-to addresses"); + + // Actions with no client equivalent. + if (CopyToFolder) f.Add("Copy to folder"); + if (SelectedMarkImportance?.Value is not null) f.Add("Set importance"); + if (SplitAddresses(ForwardTo).Count > 0) f.Add("Forward"); + if (StopProcessingRules) f.Add("Stop processing more rules"); + + // A client rule performs exactly one action. + if (ClientEligibleActionCount() > 1) f.Add("more than one action"); + + return f; + } + + /// Count of actions that a client rule could carry (it allows exactly one). + private int ClientEligibleActionCount() + { + var n = 0; + if (MarkAsRead) n++; + if (MarkAsUnread) n++; + if (MoveToFolder) n++; + if (Delete) n++; + return n; + } + + private static string Join(List items) => string.Join(", ", items); + + /// + /// 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)) || MarkAsRead + || MarkAsUnread || Delete || StopProcessingRules || !string.IsNullOrWhiteSpace(SelectedMarkImportance?.Value) @@ -232,6 +451,19 @@ private static List SplitAddresses(string text) .ToList(); } +/// +/// Result of classifying a rule (spec §20.3). Exactly one of these holds: is +/// Server; is Client with a for the save dialog; or +/// is set (the rule fits neither and must be changed before saving). +/// +public sealed record RuleClassification +{ + public RuleRunsWhere? Kind { get; init; } + public string? ClientReason { get; init; } + public string? ConflictError { get; init; } + public bool IsConflict => ConflictError is not null; +} + /// /// Importance choice for the condition/action ComboBoxes. ToString() is overridden because a /// screen reader reads a Selector item's accessible name from it, not from DisplayMemberPath. diff --git a/QuickMail/ViewModels/ServerRulesViewModel.cs b/QuickMail/ViewModels/ServerRulesViewModel.cs index 65c5c61f..1502908b 100644 --- a/QuickMail/ViewModels/ServerRulesViewModel.cs +++ b/QuickMail/ViewModels/ServerRulesViewModel.cs @@ -39,18 +39,54 @@ 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 ──────────────────────────────────────────────────────── - public ServerRulesViewModel(IServerRuleService service, IEnumerable graphAccounts) + private readonly IReadOnlyDictionary>? _foldersByAccount; + + public ServerRulesViewModel( + IServerRuleService service, + IEnumerable graphAccounts, + IReadOnlyDictionary>? foldersByAccount = null, + Guid? preferredAccountId = null) { _service = service; + _foldersByAccount = foldersByAccount; AccountOptions = graphAccounts .Where(a => a.BackendKind == BackendKind.MicrosoftGraph) .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(); + } + + /// + /// 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 ─────────────────────────────────────────────────────────────── @@ -70,8 +106,24 @@ public ServerRulesViewModel(IServerRuleService service, IEnumerableEnable/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; @@ -81,16 +133,20 @@ public ServerRulesViewModel(IServerRuleService service, IEnumerable - /// 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. @@ -112,14 +168,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) @@ -145,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.", @@ -216,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; @@ -258,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); @@ -290,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 954db921..fedfe975 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; @@ -3441,7 +3444,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. @@ -5934,7 +5943,19 @@ 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 + && _featureGate.IsEnabled(FeatureFlag.ServerRules) + && accounts.Any(a => a.BackendKind == BackendKind.MicrosoftGraph)) + // 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; // 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..e9b96bd7 100644 --- a/QuickMail/Views/RulesManagerWindow.xaml +++ b/QuickMail/Views/RulesManagerWindow.xaml @@ -36,6 +36,7 @@ + @@ -278,10 +279,107 @@ + + + + + + + + + + + + + + + +