From fc8939a3c6560fd686f2bf90e57e60d1d5f436f6 Mon Sep 17 00:00:00 2001 From: yangr-happy <301323675+yangr-happy@users.noreply.github.com> Date: Thu, 3 Sep 2026 23:30:59 +0800 Subject: [PATCH 1/2] fix: complete mail rule reorder ids --- shortcuts/mail/mail_rules.go | 65 +++++++++----- shortcuts/mail/mail_rules_test.go | 141 ++++++++++++++++++++++++++++-- 2 files changed, 179 insertions(+), 27 deletions(-) diff --git a/shortcuts/mail/mail_rules.go b/shortcuts/mail/mail_rules.go index 3a2ef5e653..2f5d46466b 100644 --- a/shortcuts/mail/mail_rules.go +++ b/shortcuts/mail/mail_rules.go @@ -370,7 +370,7 @@ var MailRuleReorder = common.Shortcut{ AuthTypes: mailRuleAuthTypes, HasFormat: true, Flags: append([]common.Flag{}, append(mailRuleCommonFlags, - common.Flag{Name: "rule-ids", Type: "string_slice", Desc: "Full target rule ID order. Must contain every current rule exactly once."}, + common.Flag{Name: "rule-ids", Type: "string_slice", Desc: "Target rule IDs to prioritize; omitted current rules are appended in their existing relative order."}, common.Flag{Name: "move-rule-id", Desc: "Rule ID to move in the current order."}, common.Flag{Name: "before-rule-id", Desc: "Place --move-rule-id before this rule."}, common.Flag{Name: "after-rule-id", Desc: "Place --move-rule-id after this rule."}, @@ -1608,6 +1608,11 @@ func validateRuleReorderFlags(rt *common.RuntimeContext) error { if full == move { return mailValidationError("exactly one of --rule-ids or --move-rule-id is required") } + if full { + if _, err := normalizeSubmittedRuleIDs(rt.StrSlice("rule-ids")); err != nil { + return err + } + } targets := 0 for _, set := range []bool{ strings.TrimSpace(rt.Str("before-rule-id")) != "", @@ -1630,11 +1635,16 @@ func validateRuleReorderFlags(rt *common.RuntimeContext) error { func buildRuleTargetOrder(rt *common.RuntimeContext, current []mailRuleEnvelope) ([]string, error) { currentIDs := envelopeRuleIDs(current) - if ids := normalizeRuleIDs(rt.StrSlice("rule-ids")); len(ids) > 0 { - if err := validateFullRuleOrder(ids, currentIDs); err != nil { + if len(rt.StrSlice("rule-ids")) > 0 { + ids, err := normalizeSubmittedRuleIDs(rt.StrSlice("rule-ids")) + if err != nil { + return nil, err + } + target, err := completeRuleTargetOrder(ids, currentIDs) + if err != nil { return nil, err } - return ids, nil + return target, nil } moveID := strings.TrimSpace(rt.Str("move-rule-id")) order := removeString(currentIDs, moveID) @@ -1653,34 +1663,49 @@ func buildRuleTargetOrder(rt *common.RuntimeContext, current []mailRuleEnvelope) } } -func validateFullRuleOrder(target, current []string) error { - if len(target) != len(current) { - return mailValidationParamError("--rule-ids", "--rule-ids must contain every current rule id exactly once (got %d, want %d)", len(target), len(current)) +func completeRuleTargetOrder(submitted, current []string) ([]string, error) { + if len(submitted) == 0 { + return nil, mailValidationParamError("--rule-ids", "--rule-ids must include at least one rule id") } - want := make(map[string]int, len(current)) + known := make(map[string]bool, len(current)) for _, id := range current { - want[id]++ + known[id] = true } - for _, id := range target { - want[id]-- + seen := make(map[string]bool, len(submitted)) + target := make([]string, 0, len(current)) + for _, id := range submitted { + if seen[id] { + return nil, mailValidationParamError("--rule-ids", "--rule-ids contains duplicate rule id %s", id) + } + if !known[id] { + return nil, mailValidationParamError("--rule-ids", "--rule-ids contains unknown rule id %s; run +rule-list first", id) + } + seen[id] = true + target = append(target, id) } - for id, count := range want { - if count != 0 { - return mailValidationParamError("--rule-ids", "--rule-ids mismatch for %s; run +rule-list first and submit the complete order", id) + for _, id := range current { + if !seen[id] { + target = append(target, id) } } - return nil + return target, nil } -func normalizeRuleIDs(ids []string) []string { +func normalizeSubmittedRuleIDs(ids []string) ([]string, error) { var out []string + seen := make(map[string]bool, len(ids)) for _, id := range ids { - id = strings.TrimSpace(id) - if id != "" { - out = append(out, id) + trimmed := strings.TrimSpace(id) + if trimmed == "" { + return nil, mailValidationParamError("--rule-ids", "--rule-ids contains an empty rule id") } + if seen[trimmed] { + return nil, mailValidationParamError("--rule-ids", "--rule-ids contains duplicate rule id %s", trimmed) + } + seen[trimmed] = true + out = append(out, trimmed) } - return out + return out, nil } func insertRelative(order []string, moveID, targetID string, after bool) ([]string, error) { diff --git a/shortcuts/mail/mail_rules_test.go b/shortcuts/mail/mail_rules_test.go index 21e5350b0e..81dfd87004 100644 --- a/shortcuts/mail/mail_rules_test.go +++ b/shortcuts/mail/mail_rules_test.go @@ -1165,6 +1165,65 @@ func TestMailRuleReorderShortcutPostsFullAndMoveOrders(t *testing.T) { assertRuleIDsBody(t, post.CapturedBody, "c,b,a") }) + t.Run("partial order completes from current order", func(t *testing.T) { + f, stdout, _, reg := mailShortcutTestFactory(t) + seenList := false + list := mailRuleListStub( + mailRuleTestRawRule("a", "A"), + mailRuleTestRawRule("b", "B"), + mailRuleTestRawRule("c", "C"), + mailRuleTestRawRule("d", "D"), + ) + list.OnMatch = func(req *http.Request) { + seenList = true + } + reg.Register(list) + post := &httpmock.Stub{ + Method: "POST", + URL: "open-apis/mail/v1/user_mailboxes/me/rules/reorder", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{}}, + } + post.OnMatch = func(req *http.Request) { + if !seenList { + t.Fatal("POST reorder was called before GET rules") + } + } + reg.Register(post) + + if err := runMountedMailShortcut(t, MailRuleReorder, []string{"+rule-reorder", "--rule-ids", " c ,a ", "--format", "json"}, f, stdout); err != nil { + t.Fatalf("run +rule-reorder partial error = %v", err) + } + if len(list.CapturedBodies) != 1 { + t.Fatalf("GET should be called once before reorder, captured %d request(s)", len(list.CapturedBodies)) + } + assertRuleIDsBody(t, post.CapturedBody, "c,a,b,d") + data := decodeShortcutEnvelopeData(t, stdout) + got, ok := data["after_rule_ids"].([]interface{}) + if !ok || strings.Join(interfaceStrings(got), ",") != "c,a,b,d" { + t.Fatalf("after_rule_ids = %v, want c,a,b,d", data["after_rule_ids"]) + } + }) + + t.Run("single id completes from current order", func(t *testing.T) { + f, stdout, _, reg := mailShortcutTestFactory(t) + reg.Register(mailRuleListStub( + mailRuleTestRawRule("a", "A"), + mailRuleTestRawRule("b", "B"), + mailRuleTestRawRule("c", "C"), + )) + post := &httpmock.Stub{ + Method: "POST", + URL: "open-apis/mail/v1/user_mailboxes/me/rules/reorder", + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{}}, + } + reg.Register(post) + + if err := runMountedMailShortcut(t, MailRuleReorder, []string{"+rule-reorder", "--rule-ids", "b", "--format", "json"}, f, stdout); err != nil { + t.Fatalf("run +rule-reorder single error = %v", err) + } + assertRuleIDsBody(t, post.CapturedBody, "b,a,c") + }) + t.Run("move to bottom", func(t *testing.T) { f, stdout, _, reg := mailShortcutTestFactory(t) reg.Register(mailRuleListStub( @@ -1488,11 +1547,20 @@ func TestMailRuleScalarHelpersCoverFallbacks(t *testing.T) { } func TestMailRuleOrderValidationErrors(t *testing.T) { - if err := validateFullRuleOrder([]string{"a"}, []string{"a", "b"}); err == nil { - t.Fatal("expected length mismatch error") + if got, err := completeRuleTargetOrder([]string{"b"}, []string{"a", "b", "c"}); err != nil || strings.Join(got, ",") != "b,a,c" { + t.Fatalf("completeRuleTargetOrder partial = %v, %v; want b,a,c", got, err) + } + if got, err := completeRuleTargetOrder([]string{"c", "a", "b"}, []string{"a", "b", "c"}); err != nil || strings.Join(got, ",") != "c,a,b" { + t.Fatalf("completeRuleTargetOrder full = %v, %v; want c,a,b", got, err) + } + if _, err := completeRuleTargetOrder([]string{"a", "a"}, []string{"a", "b"}); err == nil { + t.Fatal("expected duplicate error") } - if err := validateFullRuleOrder([]string{"a", "a"}, []string{"a", "b"}); err == nil { - t.Fatal("expected duplicate mismatch error") + if _, err := completeRuleTargetOrder([]string{"a", "z"}, []string{"a", "b"}); err == nil { + t.Fatal("expected unknown rule error") + } + if _, err := normalizeSubmittedRuleIDs([]string{"a", " "}); err == nil { + t.Fatal("expected empty rule id error") } if _, err := insertRelative([]string{"a", "b"}, "c", "", true); err == nil { t.Fatal("expected missing target error") @@ -1527,14 +1595,19 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { want: "is not in current rule order", }, { - name: "full mismatch", + name: "unknown rule id", args: []string{"+rule-reorder", "--rule-ids", "a,z"}, - want: "mismatch", + want: "unknown rule id z", + }, + { + name: "duplicate rule id", + args: []string{"+rule-reorder", "--rule-ids", "a,a"}, + want: "duplicate rule id a", }, } { t.Run(tc.name, func(t *testing.T) { f, stdout, _, reg := mailShortcutTestFactory(t) - if strings.Contains(tc.want, "current rule order") || strings.Contains(tc.want, "mismatch") { + if strings.Contains(tc.want, "current rule order") || strings.Contains(tc.want, "unknown rule id") { reg.Register(mailRuleListStub(mailRuleTestRawRule("a", "A"), mailRuleTestRawRule("b", "B"))) } err := runMountedMailShortcut(t, MailRuleReorder, append(tc.args, "--format", "json"), f, stdout) @@ -1548,6 +1621,52 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { } } +func TestMailRuleReorderDoesNotPostWhenListOrLocalValidationFails(t *testing.T) { + t.Run("list failure", func(t *testing.T) { + f, stdout, _, reg := mailShortcutTestFactory(t) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "open-apis/mail/v1/user_mailboxes/me/rules", + Body: map[string]interface{}{"code": 12345, "msg": "list failed"}, + }) + post := &httpmock.Stub{ + Method: "POST", + URL: "open-apis/mail/v1/user_mailboxes/me/rules/reorder", + Optional: true, + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{}}, + } + reg.Register(post) + + err := runMountedMailShortcut(t, MailRuleReorder, []string{"+rule-reorder", "--rule-ids", "a", "--format", "json"}, f, stdout) + if err == nil || !strings.Contains(err.Error(), "list mail rules before reorder failed") { + t.Fatalf("error = %v, want list failure", err) + } + if len(post.CapturedBodies) != 0 { + t.Fatalf("POST should not be sent after list failure, captured %d request(s)", len(post.CapturedBodies)) + } + }) + + t.Run("unknown rule", func(t *testing.T) { + f, stdout, _, reg := mailShortcutTestFactory(t) + reg.Register(mailRuleListStub(mailRuleTestRawRule("a", "A"))) + post := &httpmock.Stub{ + Method: "POST", + URL: "open-apis/mail/v1/user_mailboxes/me/rules/reorder", + Optional: true, + Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{}}, + } + reg.Register(post) + + err := runMountedMailShortcut(t, MailRuleReorder, []string{"+rule-reorder", "--rule-ids", "z", "--format", "json"}, f, stdout) + if err == nil || !strings.Contains(err.Error(), "unknown rule id z") { + t.Fatalf("error = %v, want unknown rule validation", err) + } + if len(post.CapturedBodies) != 0 { + t.Fatalf("POST should not be sent after local validation failure, captured %d request(s)", len(post.CapturedBodies)) + } + }) +} + func mailRuleTestRawRule(ruleID, name string) map[string]interface{} { return map[string]interface{}{ "rule_id": ruleID, @@ -1613,3 +1732,11 @@ func assertRuleIDsBody(t *testing.T, raw []byte, want string) { t.Fatalf("rule_ids = %v, want %s", got, want) } } + +func interfaceStrings(items []interface{}) []string { + out := make([]string, 0, len(items)) + for _, item := range items { + out = append(out, item.(string)) + } + return out +} From ea669de03548160092f6de713536fb3c07a7a312 Mon Sep 17 00:00:00 2001 From: yangr-happy <301323675+yangr-happy@users.noreply.github.com> Date: Thu, 3 Sep 2026 23:54:46 +0800 Subject: [PATCH 2/2] test: assert mail rule reorder typed errors Change-Type: ci-fix --- shortcuts/mail/mail_rules_test.go | 57 ++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/shortcuts/mail/mail_rules_test.go b/shortcuts/mail/mail_rules_test.go index 81dfd87004..54e23b98c1 100644 --- a/shortcuts/mail/mail_rules_test.go +++ b/shortcuts/mail/mail_rules_test.go @@ -1555,12 +1555,18 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { } if _, err := completeRuleTargetOrder([]string{"a", "a"}, []string{"a", "b"}); err == nil { t.Fatal("expected duplicate error") + } else { + assertMailRuleValidationProblem(t, err, "--rule-ids") } if _, err := completeRuleTargetOrder([]string{"a", "z"}, []string{"a", "b"}); err == nil { t.Fatal("expected unknown rule error") + } else { + assertMailRuleValidationProblem(t, err, "--rule-ids") } if _, err := normalizeSubmittedRuleIDs([]string{"a", " "}); err == nil { t.Fatal("expected empty rule id error") + } else { + assertMailRuleValidationProblem(t, err, "--rule-ids") } if _, err := insertRelative([]string{"a", "b"}, "c", "", true); err == nil { t.Fatal("expected missing target error") @@ -1570,9 +1576,10 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { } for _, tc := range []struct { - name string - args []string - want string + name string + args []string + want string + wantParam string }{ { name: "no mode", @@ -1595,14 +1602,16 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { want: "is not in current rule order", }, { - name: "unknown rule id", - args: []string{"+rule-reorder", "--rule-ids", "a,z"}, - want: "unknown rule id z", + name: "unknown rule id", + args: []string{"+rule-reorder", "--rule-ids", "a,z"}, + want: "unknown rule id z", + wantParam: "--rule-ids", }, { - name: "duplicate rule id", - args: []string{"+rule-reorder", "--rule-ids", "a,a"}, - want: "duplicate rule id a", + name: "duplicate rule id", + args: []string{"+rule-reorder", "--rule-ids", "a,a"}, + want: "duplicate rule id a", + wantParam: "--rule-ids", }, } { t.Run(tc.name, func(t *testing.T) { @@ -1614,6 +1623,7 @@ func TestMailRuleOrderValidationErrors(t *testing.T) { if err == nil { t.Fatal("expected reorder error") } + assertMailRuleValidationProblem(t, err, tc.wantParam) if !strings.Contains(err.Error(), tc.want) { t.Fatalf("error = %v, want %q", err, tc.want) } @@ -1641,6 +1651,11 @@ func TestMailRuleReorderDoesNotPostWhenListOrLocalValidationFails(t *testing.T) if err == nil || !strings.Contains(err.Error(), "list mail rules before reorder failed") { t.Fatalf("error = %v, want list failure", err) } + assertMailRuleProblem(t, err, errs.CategoryAPI, errs.SubtypeUnknown) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("expected decorated error to preserve APIError cause, got %T: %v", err, err) + } if len(post.CapturedBodies) != 0 { t.Fatalf("POST should not be sent after list failure, captured %d request(s)", len(post.CapturedBodies)) } @@ -1661,12 +1676,36 @@ func TestMailRuleReorderDoesNotPostWhenListOrLocalValidationFails(t *testing.T) if err == nil || !strings.Contains(err.Error(), "unknown rule id z") { t.Fatalf("error = %v, want unknown rule validation", err) } + assertMailRuleValidationProblem(t, err, "--rule-ids") if len(post.CapturedBodies) != 0 { t.Fatalf("POST should not be sent after local validation failure, captured %d request(s)", len(post.CapturedBodies)) } }) } +func assertMailRuleProblem(t testing.TB, err error, wantCategory errs.Category, wantSubtype errs.Subtype) { + t.Helper() + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("ProblemOf(%T) ok = false, want true: %v", err, err) + } + if p.Category != wantCategory || p.Subtype != wantSubtype { + t.Fatalf("problem = %s/%s, want %s/%s", p.Category, p.Subtype, wantCategory, wantSubtype) + } +} + +func assertMailRuleValidationProblem(t testing.TB, err error, wantParam string) { + t.Helper() + assertMailRuleProblem(t, err, errs.CategoryValidation, errs.SubtypeInvalidArgument) + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("expected ValidationError, got %T: %v", err, err) + } + if wantParam != "" && validationErr.Param != wantParam { + t.Fatalf("validation Param = %q, want %q", validationErr.Param, wantParam) + } +} + func mailRuleTestRawRule(ruleID, name string) map[string]interface{} { return map[string]interface{}{ "rule_id": ruleID,