Complete mail rule reorder IDs - #2167
Conversation
Fetch the current mail rule order before submitting reorder requests so partial input can be completed locally and invalid IDs fail before the write call. Co-authored-by: TRAE CLI <noreply@bytedance.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe service now completes mail rule reorder requests before execution. It validates rule IDs, fetches current rules for partial orders, preserves requested order, and propagates validation or list API errors. ChangesMail rules reorder
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Service
participant MailRulesReorder
participant MailRulesAPI
Service->>MailRulesReorder: Match and validate reorder request
MailRulesReorder->>MailRulesAPI: GET current mail rules
MailRulesAPI-->>MailRulesReorder: Return rule IDs
MailRulesReorder->>MailRulesReorder: Complete rule_ids order
MailRulesReorder-->>Service: Return updated request
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d6bf3875f6a6590787da31dcdcec9c8126272bb9🧩 Skill updatenpx skills add yangr-happy/cli#feat/de08ac7 -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/service/service.go (1)
404-425: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
--dry-runbypasses reorder completion and validation.
completeMailRulesReorderruns at Line 422, after the--dry-runearly return at Lines 404-409. For a mail rules reorder command,--dry-runtherefore prints the raw, unfinishedrule_idsinstead of the request that would actually be submitted, and skips duplicate/unknown-ID validation entirely.Move the API client creation and
completeMailRulesReordercall before the DryRun check so the preview reflects the completed and validated request.🐛 Proposed fix to run completion before the DryRun preview
request, fileMeta, err := buildServiceRequest(opts) if err != nil { return err } - if opts.DryRun { - if fileMeta != nil { - return cmdutil.PrintDryRunWithFile(f.IOStreams.Out, request, config, opts.Format, fileMeta.FieldName, fileMeta.FilePath, fileMeta.FormFields) - } - return serviceDryRun(f, request, config, opts.Format) - } - - if opts.Method.Risk == cmdutil.RiskHighRiskWrite { - if yes, _ := opts.Cmd.Flags().GetBool("yes"); !yes { - return cmdutil.RequireConfirmation(opts.SchemaPath) - } - } - ac, err := f.NewAPIClientWithConfig(config) if err != nil { return err } if err := completeMailRulesReorder(opts.Ctx, ac, &request, opts.SchemaPath, opts.As); err != nil { return err } + if opts.DryRun { + if fileMeta != nil { + return cmdutil.PrintDryRunWithFile(f.IOStreams.Out, request, config, opts.Format, fileMeta.FieldName, fileMeta.FilePath, fileMeta.FormFields) + } + return serviceDryRun(f, request, config, opts.Format) + } + + if opts.Method.Risk == cmdutil.RiskHighRiskWrite { + if yes, _ := opts.Cmd.Flags().GetBool("yes"); !yes { + return cmdutil.RequireConfirmation(opts.SchemaPath) + } + } + out := f.IOStreams.Out🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/service/service.go` around lines 404 - 425, Move the API client creation and completeMailRulesReorder call in the command flow before the opts.DryRun early return, while preserving the existing high-risk confirmation and preview behavior. Ensure dry-run mail rules reorder requests use the completed, validated request so duplicate and unknown rule IDs are rejected and the preview reflects the submitted request.
🧹 Nitpick comments (1)
cmd/service/service_test.go (1)
583-625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining
rule_idsvalidation branches.Tests cover duplicate and unknown rule IDs, but not two other new validation branches in
mail_rules_reorder.go:
--datapayload that is not a JSON object (Lines 24-27 of mail_rules_reorder.go).rule_idsthat is not an array, or contains a non-string/empty element (Lines 138-146 of mail_rules_reorder.go).Add test cases for these branches to lock in the typed-error contract for each.
As per coding guidelines, "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/service/service_test.go` around lines 583 - 625, Extend the service reorder tests around TestServiceMethod_MailRulesReorderRejectsDuplicateIDs to cover a non-object --data payload and invalid rule_ids values: a non-array, a non-string element, and an empty string element. Execute each through mailRulesReorderMethod and assert the typed validation error contract directly with requireProblem, preserving the existing duplicate and unknown-ID coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/service/mail_rules_reorder.go`:
- Around line 126-147: Update stringSliceField so the []string branch validates
every element as a non-empty string, matching the existing []any validation and
returning the same indexed validation error for empty values; preserve copying
valid slices and the existing handling for missing or invalid fields.
- Around line 82-124: Update listMailRuleIDs to retrieve every mail rule through
the list endpoint’s pagination mechanism, carrying page_size/page_token until no
next-page token remains before building the ID list. Preserve response
validation for each page and ensure IDs retain server-provided order; if
pagination metadata or ordering semantics are unavailable, return a clear
invalid-response error rather than submitting a partial reorder.
---
Outside diff comments:
In `@cmd/service/service.go`:
- Around line 404-425: Move the API client creation and completeMailRulesReorder
call in the command flow before the opts.DryRun early return, while preserving
the existing high-risk confirmation and preview behavior. Ensure dry-run mail
rules reorder requests use the completed, validated request so duplicate and
unknown rule IDs are rejected and the preview reflects the submitted request.
---
Nitpick comments:
In `@cmd/service/service_test.go`:
- Around line 583-625: Extend the service reorder tests around
TestServiceMethod_MailRulesReorderRejectsDuplicateIDs to cover a non-object
--data payload and invalid rule_ids values: a non-array, a non-string element,
and an empty string element. Execute each through mailRulesReorderMethod and
assert the typed validation error contract directly with requireProblem,
preserving the existing duplicate and unknown-ID coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dcec4fcc-f280-4d81-850f-928c9782c608
📒 Files selected for processing (3)
cmd/service/mail_rules_reorder.gocmd/service/service.gocmd/service/service_test.go
Fetch every mail rule page before completing partial reorder inputs, and validate empty rule IDs consistently. Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/service/mail_rules_reorder.go (1)
101-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRead
has_morefrom the object that containsitems.Line 110 reads only top-level
has_more. The normal response shape incmd/service/service_test.goLines 559-566 stores it indata.has_more. This guard can therefore accept an incomplete nested list response and submit a reorder that omits rules.Proposed fix
- items, ok := data["items"].([]any) + items, ok := data["items"].([]any) + pagination := data if !ok { if nested, hasData := data["data"].(map[string]any); hasData { items, ok = nested["items"].([]any) + pagination = nested } } @@ - if hasMore, _ := data["has_more"].(bool); hasMore { + if hasMore, _ := pagination["has_more"].(bool); hasMore { return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "mail rules list pagination did not return all pages") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/service/mail_rules_reorder.go` around lines 101 - 112, Update the mail rules response parsing around the items extraction to track whether items came from the top-level data object or the nested data object, then read has_more from that same containing object. Keep rejecting responses where has_more is true, including nested responses, before returning the parsed items.cmd/service/service_test.go (2)
699-731: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that the reorder request was not sent.
These tests only assert that
cmd.Executereturns an error. A faulty implementation can attempt the POST and still pass because the mock has no matching reorder stub. Register a reorder stub that records matches, then assert that it was not called after both list failure and an empty list.As per coding guidelines, contract tests must assert the changed behavior directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/service/service_test.go` around lines 699 - 731, Update both MailRules reorder tests, TestServiceMethod_MailRulesReorderListFailureSkipsReorder and TestServiceMethod_MailRulesReorderEmptyListSkipsReorder, to register a reorder POST stub that records whether it matches. After cmd.Execute, retain the existing error assertions and additionally assert that the reorder stub was not called in either failure path.Source: Coding guidelines
639-697: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest the
[]stringvalidation path and assertParam.
cmd.Executedecodes JSON arrays as[]any. These tests do not exercise the new[]stringbranch instringSliceField. Add a direct test with[]string{""}. Also assertValidationError.Param == "rule_ids"througherrors.As.As per coding guidelines, error-path tests must assert typed metadata. Based on learnings, use
errors.AsforParambecauseerrs.ProblemOfdoes not expose it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/service/service_test.go` around lines 639 - 697, Extend the mail-rules validation tests to directly exercise the []string branch in stringSliceField by adding a case that passes []string{""} and assert the resulting ValidationError via errors.As, including Param == "rule_ids". Update the existing error-path assertions as needed to verify this typed metadata while preserving the current problem category, subtype, and message checks.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/service/mail_rules_reorder.go`:
- Around line 101-112: Update the mail rules response parsing around the items
extraction to track whether items came from the top-level data object or the
nested data object, then read has_more from that same containing object. Keep
rejecting responses where has_more is true, including nested responses, before
returning the parsed items.
In `@cmd/service/service_test.go`:
- Around line 699-731: Update both MailRules reorder tests,
TestServiceMethod_MailRulesReorderListFailureSkipsReorder and
TestServiceMethod_MailRulesReorderEmptyListSkipsReorder, to register a reorder
POST stub that records whether it matches. After cmd.Execute, retain the
existing error assertions and additionally assert that the reorder stub was not
called in either failure path.
- Around line 639-697: Extend the mail-rules validation tests to directly
exercise the []string branch in stringSliceField by adding a case that passes
[]string{""} and assert the resulting ValidationError via errors.As, including
Param == "rule_ids". Update the existing error-path assertions as needed to
verify this typed metadata while preserving the current problem category,
subtype, and message checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4a919a1-b6b4-45ac-b402-d73c281291dc
📒 Files selected for processing (2)
cmd/service/mail_rules_reorder.gocmd/service/service_test.go
Use the rule list response id field when completing reorder requests so partial input can be expanded before calling the reorder endpoint. Co-authored-by: TRAE CLI <noreply@bytedance.com>
|
🤖 AI Review | CR 汇总 | 有风险（0 个新增评论，2 个已由既有评论覆盖） 增量审查：已读取既有评论并按同位置同问题去重。本轮确认 验证： |
1 participant
Fetch the current mail rule order before submitting reorder requests.
Summary by CodeRabbit
New Features
Bug Fixes