feat(base): add button rule commands - #2289
Conversation
📝 WalkthroughWalkthroughChangesButton workflow binding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ButtonShortcut
participant BaseV3API
CLI->>ButtonShortcut: Run bind, get, or unbind
ButtonShortcut->>ButtonShortcut: Validate Base, table, field, and workflow IDs
ButtonShortcut->>BaseV3API: Send PUT, GET, or DELETE button_rule request
BaseV3API-->>ButtonShortcut: Return rule data or error
ButtonShortcut-->>CLI: Report status and result
Possibly related PRs
Suggested reviewers: 🚥 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
domain/base
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@shortcuts/base/base_dryrun_ops_test.go`:
- Around line 122-138: Add mounted shortcut-level dry-run E2E coverage for the
button-rule shortcut, exercising mounting, flag wiring, validation,
confirmation, and Execute request plumbing instead of calling
dryRunButtonBind/Get/Unbind directly. Add self-contained live E2E coverage that
creates the workflow and button field, binds and retrieves the rule, unbinds it,
and cleans up all created resources.
- Around line 151-159: The validation tests around validateButtonBind and
validateButtonRuleLocator currently assert only message text; update them to
type-assert the declared validation error and verify its category, subtype, and
Param values for --workflow-id and --field-id. Where the validation path wraps
an underlying cause, also assert that the cause is preserved, while removing
reliance on strings.Contains as the primary assertion.
In `@shortcuts/base/button_rule.go`:
- Around line 107-113: Ensure validateButtonBind, dryRunButtonBind, and
executeButtonBind consistently handle surrounding whitespace in workflow-id:
either reject padded input during validation or propagate the trimmed workflowID
into both request bodies. Add a regression test covering whitespace-padded
workflow IDs and verify the API never receives the raw spaced value.
In `@shortcuts/base/helpers.go`:
- Around line 178-179: Add nearby table-driven regression tests targeting
resolveFieldTypeSpec, with separate cases for button, buttonfield, button_field,
and button-field. Assert each alias resolves to the button field type, ensuring
reverting the alias handling causes the tests to fail.
🪄 Autofix
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: 85bed815-db1b-4691-912d-d09d5afc1232
📒 Files selected for processing (7)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/button_rule.goshortcuts/base/helpers.goshortcuts/base/shortcuts.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-field-json.md
| func TestDryRunButtonRuleOps(t *testing.T) { | ||
| ctx := context.Background() | ||
| rt := newBaseTestRuntime( | ||
| map[string]string{ | ||
| "base-token": "app_x", | ||
| "table-id": "tbl_1", | ||
| "field-id": "fld_button", | ||
| "workflow-id": "wkf_1", | ||
| }, | ||
| nil, | ||
| nil, | ||
| ) | ||
|
|
||
| assertDryRunContains(t, dryRunButtonBind(ctx, rt), "PUT /open-apis/base/v3/bases/app_x/tables/tbl_1/fields/fld_button/button_rule", `"workflow_id":"wkf_1"`) | ||
| assertDryRunContains(t, dryRunButtonGet(ctx, rt), "GET /open-apis/base/v3/bases/app_x/tables/tbl_1/fields/fld_button/button_rule") | ||
| assertDryRunContains(t, dryRunButtonUnbind(ctx, rt), "DELETE /open-apis/base/v3/bases/app_x/tables/tbl_1/fields/fld_button/button_rule") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add mounted dry-run and live E2E coverage for the new shortcuts.
This test calls dryRunButtonBind, dryRunButtonGet, and dryRunButtonUnbind directly. It does not cover shortcut mounting, flag wiring, validation routing, confirmation behavior, or Execute request plumbing.
Add shortcut-level dry-run E2E tests. Add self-contained live E2E coverage that creates the workflow and button field, binds, gets, unbinds, and cleans up the resources.
As per coding guidelines: “Shortcut changes require dry-run E2E coverage” and “new shortcuts require live E2E coverage.”
🤖 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 `@shortcuts/base/base_dryrun_ops_test.go` around lines 122 - 138, Add mounted
shortcut-level dry-run E2E coverage for the button-rule shortcut, exercising
mounting, flag wiring, validation, confirmation, and Execute request plumbing
instead of calling dryRunButtonBind/Get/Unbind directly. Add self-contained live
E2E coverage that creates the workflow and button field, binds and retrieves the
rule, unbinds it, and cleans up all created resources.
Source: Coding guidelines
| if err := validateButtonBind(badWorkflow); err == nil || !strings.Contains(err.Error(), "wkf prefix") { | ||
| t.Fatalf("expected public workflow ID validation error, got %v", err) | ||
| } | ||
|
|
||
| missingField := newBaseTestRuntime(map[string]string{ | ||
| "base-token": "app_x", "table-id": "tbl_1", "workflow-id": "wkfAbcdefg", | ||
| }, nil, nil) | ||
| if err := validateButtonRuleLocator(missingField); err == nil || !strings.Contains(err.Error(), "--field-id") { | ||
| t.Fatalf("expected missing field validation error, got %v", err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert typed validation error metadata.
These checks only inspect err.Error(). They pass if the validation path regresses from a typed error to a plain error.
Assert the declared typed validation error, its category, subtype, and Param for --workflow-id and --field-id. Preserve and assert a cause when the tested path has one.
As per coding guidelines: “Error tests must assert typed metadata and cause preservation rather than message text alone.”
🤖 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 `@shortcuts/base/base_dryrun_ops_test.go` around lines 151 - 159, The
validation tests around validateButtonBind and validateButtonRuleLocator
currently assert only message text; update them to type-assert the declared
validation error and verify its category, subtype, and Param values for
--workflow-id and --field-id. Where the validation path wraps an underlying
cause, also assert that the cause is preserved, while removing reliance on
strings.Contains as the primary assertion.
Source: Coding guidelines
| workflowID := strings.TrimSpace(runtime.Str("workflow-id")) | ||
| if workflowID == "" { | ||
| return baseFlagErrorf("--workflow-id must not be blank") | ||
| } | ||
| if !strings.HasPrefix(workflowID, "wkf") { | ||
| return baseFlagErrorf("--workflow-id must be a public workflow ID with wkf prefix") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject or normalize surrounding whitespace in workflow-id.
validateButtonBind accepts " wkf_1 " after trimming it. dryRunButtonBind and executeButtonBind then send the raw value with spaces. The API receives an invalid workflow ID.
Reject leading or trailing whitespace before validation succeeds, or use the normalized value in both request bodies. Add a regression test for whitespace-padded input.
Also applies to: 126-134, 152-159
🤖 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 `@shortcuts/base/button_rule.go` around lines 107 - 113, Ensure
validateButtonBind, dryRunButtonBind, and executeButtonBind consistently handle
surrounding whitespace in workflow-id: either reject padded input during
validation or propagate the trimmed workflowID into both request bodies. Add a
regression test covering whitespace-padded workflow IDs and verify the API never
receives the raw spaced value.
| case "button", "buttonfield", "button_field", "button-field": | ||
| return fieldTypeSpec{Type: "button"}, nil |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add regression coverage for each button alias.
Add table-driven cases for button, buttonfield, button_field, and button-field. The supplied test changes do not verify resolveFieldTypeSpec, so an alias can regress without detection.
As per coding guidelines: “Every behavior change requires a nearby regression test that fails when the implementation is reverted.”
🤖 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 `@shortcuts/base/helpers.go` around lines 178 - 179, Add nearby table-driven
regression tests targeting resolveFieldTypeSpec, with separate cases for button,
buttonfield, button_field, and button-field. Assert each alias resolves to the
button field type, ensuring reverting the alias handling causes the tests to
fail.
Source: Coding guidelines
|
wanghaomin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
2 participants
Summary
Test
Summary by CodeRabbit
New Features
Documentation