From 0ebaa51cbd6f540194fa229aaf34fee91fd8a37d Mon Sep 17 00:00:00 2001 From: Rogee Date: Sat, 6 Jun 2026 02:41:31 +0800 Subject: [PATCH] feat(agents): align update validation errors --- docs/CHATWOOT_PARITY_DEVELOPMENT_PLAN.md | 10 +++--- internal/handler/api/v1/agent_handler.go | 7 ++++ internal/handler/api/v1/agent_handler_test.go | 33 +++++++++++++++++++ internal/service/agent_service.go | 8 +++++ 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/docs/CHATWOOT_PARITY_DEVELOPMENT_PLAN.md b/docs/CHATWOOT_PARITY_DEVELOPMENT_PLAN.md index 42a51057..db802a39 100644 --- a/docs/CHATWOOT_PARITY_DEVELOPMENT_PLAN.md +++ b/docs/CHATWOOT_PARITY_DEVELOPMENT_PLAN.md @@ -16,10 +16,10 @@ Build GoChat as a Go backend that can directly reuse the frontend from `referenc ## Current Baseline -- Current tracking checkpoint: 2026-06-06 after `8b30cc7 feat(agents): align duplicate create errors`, with this implementation checkpoint prepared as `feat(agents): align delete scope`. -- Latest implementation checkpoint: this checkpoint, prepared as `feat(agents): align delete scope`. +- Current tracking checkpoint: 2026-06-06 after `ec1bb25 feat(agents): align delete scope`, with this implementation checkpoint prepared as `feat(agents): align update validation errors`. +- Latest implementation checkpoint: this checkpoint, prepared as `feat(agents): align update validation errors`. - Latest documentation-only checkpoint: `8b378c7 docs: refresh parity tracker baseline`; this document is now the active follow-up plan and supersedes `.hermes/plans/*`. -- Worktree status at this implementation checkpoint: Agent delete now matches Chatwoot's `agents.find(params[:id])` account scope: deleting a user who is not a member of the current account returns `404` and does not remove that user or any other account membership. Duplicate account agent creation already matches Chatwoot's `ActiveRecord::RecordInvalid` response shape: `422` with `{ message, attributes }` instead of a local `409` envelope. Agent creation already matches Chatwoot `AgentBuilder` name fallback: blank or omitted `agent[name]` no longer fails local validation and new users receive the email local-part as `name`. Account conversation/contact attachment list pagination already matches Chatwoot's fixed 100-row page size for `ConversationsController#attachments` and `Contacts::AttachmentsController#index`; reused frontend `per_page` query values no longer shrink shared attachment payloads below the reference page size. Profile account permission drift is closed for reused route guards and dashboard permission helpers: profile account entries now return Chatwoot `AccountUser#permissions` for ordinary roles (`["administrator"]` or `["agent"]`) and continue to return enterprise custom-role permissions plus `custom_role` when `custom_role_id` is present. The preceding platform account-user membership, agents list ordering, update-route/`auto_offline`, agents bulk invite, B10.3 CustomRole frontend update, and Phase 6 contacts pagination/active payload drifts are closed. Conversation transcript and custom-attribute response drifts are already closed. The fresh placeholder audit found remaining `chatwootParityStub` routes only in webhook handler-not-injected fallbacks, not in the reused dashboard account/contact/conversation/message/inbox critical path. P3.6 custom filters/custom attribute definitions and label CRUD are already in Review. B11.1a-B11.3e Captain/Copilot slices remain in Review; B12.1/B12.2/B12.3 smoke harnesses remain in Review; P5 durable job work through P5.13b is in Review. Next active implementation slice is the next named Phase 2/3 or Phase 6 drift from fresh reference/smoke evidence. +- Worktree status at this implementation checkpoint: Agent update blank-name validation now matches Chatwoot `User#update!` plus `RequestExceptionHandler#render_record_invalid`: explicit blank `agent[name]` returns `422 { message: "Name can't be blank", attributes: ["name"] }` and leaves the user name unchanged. Agent delete already matches Chatwoot's `agents.find(params[:id])` account scope: deleting a user who is not a member of the current account returns `404` and does not remove that user or any other account membership. Duplicate account agent creation already matches Chatwoot's `ActiveRecord::RecordInvalid` response shape: `422` with `{ message, attributes }` instead of a local `409` envelope. Agent creation already matches Chatwoot `AgentBuilder` name fallback: blank or omitted `agent[name]` no longer fails local validation and new users receive the email local-part as `name`. Account conversation/contact attachment list pagination already matches Chatwoot's fixed 100-row page size for `ConversationsController#attachments` and `Contacts::AttachmentsController#index`; reused frontend `per_page` query values no longer shrink shared attachment payloads below the reference page size. Profile account permission drift is closed for reused route guards and dashboard permission helpers: profile account entries now return Chatwoot `AccountUser#permissions` for ordinary roles (`["administrator"]` or `["agent"]`) and continue to return enterprise custom-role permissions plus `custom_role` when `custom_role_id` is present. The preceding platform account-user membership, agents list ordering, update-route/`auto_offline`, agents bulk invite, B10.3 CustomRole frontend update, and Phase 6 contacts pagination/active payload drifts are closed. Conversation transcript and custom-attribute response drifts are already closed. The fresh placeholder audit found remaining `chatwootParityStub` routes only in webhook handler-not-injected fallbacks, not in the reused dashboard account/contact/conversation/message/inbox critical path. P3.6 custom filters/custom attribute definitions and label CRUD are already in Review. B11.1a-B11.3e Captain/Copilot slices remain in Review; B12.1/B12.2/B12.3 smoke harnesses remain in Review; P5 durable job work through P5.13b is in Review. Next active implementation slice is the next named Phase 2/3 or Phase 6 drift from fresh reference/smoke evidence. - `go test ./...` passes. - Route dump succeeds with `TOTAL: 840` after adding the Chatwoot platform account-user collection destroy route. - Route parity artifacts now exist under `docs/parity/` and are generated by `cmd/route_parity`. @@ -87,7 +87,7 @@ This table is the shortest authoritative handoff view. If an older lower section | Priority | Workstream | Current state | Next checkpoint | Commit close rule | | --- | --- | --- | --- | --- | -| 1 | Phase 2/3 drift | Tracked route parity is 0 missing for the current critical set; notification list/action serializers, Devise password reset/confirmation payloads, CRM shared attachment payloads plus fixed 100-row attachment pagination, account/settings payloads, assignable-agent payloads, agent create/delete defaults/errors/scope, label CRUD payloads, custom filters, and custom attribute definitions now match the inspected Chatwoot frontend contract. | Continue Phase 6 placeholder audit, then open the next named serializer/controller drift slice from reference/smoke evidence. | Regenerate parity artifacts when routes change and add endpoint-family fixture tests. | +| 1 | Phase 2/3 drift | Tracked route parity is 0 missing for the current critical set; notification list/action serializers, Devise password reset/confirmation payloads, CRM shared attachment payloads plus fixed 100-row attachment pagination, account/settings payloads, assignable-agent payloads, agent create/update/delete defaults/errors/scope, label CRUD payloads, custom filters, and custom attribute definitions now match the inspected Chatwoot frontend contract. | Continue Phase 6 placeholder audit, then open the next named serializer/controller drift slice from reference/smoke evidence. | Regenerate parity artifacts when routes change and add endpoint-family fixture tests. | | 2 | Phase 6 placeholder audit | Widget/public/webhook critical placeholders are burned down; inbox WhatsApp health/register-webhook and sync-template drift are closed; fresh `rg` audit shows only webhook nil-handler fallbacks still call `chatwootParityStub`; dashboard conversation transcript response drift is closed. | Continue targeted account/contact/conversation/message/inbox drift from reference/smoke evidence. | `rg` placeholder audit is recorded and no reused-frontend blocker is ownerless. | | 3 | B12 optional live smoke | API/browser/enterprise smoke commands are checked in; live runs need PostgreSQL, Redis, Meilisearch, Vite, and Chrome. | Run full live smoke when environment is available and map failures to the board. | `docs/parity/frontend_smoke_report.md` records pass/fail and linked owners. | | 4 | B9.3 delayed automation actions | Current reference exposes no delayed automation action params; scheduled-item work is already P5.12; `send_email_to_team` is durable and `add_sla` mutates conversation/applied SLA state. | Keep automation drift closed if future reference/smoke exposes delayed params or unsupported action shapes. | Automation worker/action fixtures verify queued team email replay, retry visibility through worker jobs, and SLA action idempotency. | @@ -143,6 +143,7 @@ This ledger records the committed parity checkpoints that future slices should b | Commit | Scope | Verification summary | Follow-up state | | --- | --- | --- | --- | +| `feat(agents): align update validation errors` | Advances P3.2 agents/settings parity by matching Chatwoot `AgentsController#update`, `User#update!`, and `RequestExceptionHandler#render_record_invalid`. Explicit blank `agent[name]` updates now return `422 { message: "Name can't be blank", attributes: ["name"] }` and preserve the existing user name instead of silently ignoring the invalid value with a `200 OK`. | `go test ./internal/handler/api/v1 -run 'TestAgentHandlerSuite/TestUpdateAgent' -count=1`; `go test ./internal/service -run Agent -count=1`; `go test ./internal/handler/api/v1 ./internal/service ./internal/repository -count=1`; `go test ./...`; `git diff --check`. No route changes; route dump remains `TOTAL: 840`. | Continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. | | `feat(agents): align delete scope` | Advances P3.2 agents/settings parity by matching Chatwoot `AgentsController#fetch_agent` before `destroy`: deletes are scoped through current account membership and return `404` when the target user is not an agent in the account. The local delete path now looks up the `AccountUser` before deletion so cross-account/non-member users are not accidentally deleted. | `go test ./internal/handler/api/v1 -run 'TestAgentHandlerSuite/TestDeleteAgent' -count=1`; `go test ./internal/handler/api/v1 ./internal/service ./internal/repository -count=1`; `go test ./...`; `git diff --check`. No route changes; route dump remains `TOTAL: 840`. | Continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. | | `feat(agents): align duplicate create errors` | Advances P3.2 agents/settings parity by matching Chatwoot's `ActiveRecord::RecordInvalid` handling for duplicate account memberships created through `AgentsController#create`. Re-inviting an existing account member now returns `422 { message: "User has already been taken", attributes: ["user_id"] }` instead of the local `409` conflict envelope, while bulk invite continues to skip duplicate failures like the reference controller. | `go test ./internal/handler/api/v1 -run 'TestAgentHandlerSuite/TestCreateAgentDuplicate' -count=1`; `go test ./internal/handler/api/v1 ./internal/service ./internal/repository -count=1`; `go test ./...`; `git diff --check`. No route changes; route dump remains `TOTAL: 840`. | Continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. | | `feat(agents): align create name defaults` | Advances P3.2 agents/settings parity by matching Chatwoot `AgentsController#create` and `AgentBuilder`. Agent creation now accepts blank or omitted `agent[name]`; for newly created users, the name falls back to the email local-part exactly like `email.split('@').first`, while existing-user invitation behavior remains unchanged. | `go test ./internal/handler/api/v1 -run 'TestAgentHandlerSuite/TestCreateAgent' -count=1`; `go test ./internal/service ./internal/repository -run Agent -count=1`; `go test ./internal/handler/api/v1 ./internal/service ./internal/repository -count=1`; `go test ./...`; `git diff --check`. No route changes; route dump remains `TOTAL: 840`. | Continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. | @@ -2081,6 +2082,7 @@ Verification milestone gates: ## Progress Log +- 2026-06-06: P3.2 agent update-validation checkpoint prepared as `feat(agents): align update validation errors`; audited Chatwoot `AgentsController#update`, `User` name presence validation, and `RequestExceptionHandler#render_record_invalid`. Explicit blank `agent[name]` updates now return Chatwoot `422 { message, attributes }`, preserve the existing user name, and continue to allow omitted names plus explicit `auto_offline: false`. Focused update tests, service agent tests, combined handler/service/repository tests, full `go test ./...`, and `git diff --check` passed; continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. - 2026-06-06: P3.2 agent delete-scope checkpoint prepared as `feat(agents): align delete scope`; audited Chatwoot `AgentsController#destroy`, `fetch_agent`, and account-scoped `agents` relation. Agent delete now first resolves the current account membership and returns `404` for non-members, preserving users and memberships in other accounts instead of treating an unscoped delete as success. Focused delete tests, combined handler/service/repository tests, full `go test ./...`, and `git diff --check` passed; continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. - 2026-06-06: P3.2 agent duplicate-create checkpoint prepared as `feat(agents): align duplicate create errors`; audited Chatwoot `AgentsController#create`, `AgentBuilder#create_account_user`, `AccountUser` uniqueness validation, and `RequestExceptionHandler#render_record_invalid`. Duplicate account membership creation through `/agents` now returns Chatwoot `422 { message, attributes }` instead of local `409`, while bulk invite duplicate skipping remains unchanged. Focused duplicate-create tests, combined handler/service/repository tests, full `go test ./...`, and `git diff --check` passed; continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. - 2026-06-06: P3.2 agent create-default checkpoint prepared as `feat(agents): align create name defaults`; audited Chatwoot `AgentsController#create`, `AgentBuilder`, `AccountUser`, and the reused dashboard agents API. Agent creation now accepts blank/omitted `agent[name]`, derives new-user names from the email local-part like `email.split('@').first`, and keeps existing agent create role/availability/auto-offline behavior intact. Focused agent create tests, service/repository agent tests, combined handler/service/repository tests, full `go test ./...`, and `git diff --check` passed; continue P3.2 agents/account-user drift or the next Phase 6 reference/smoke finding. diff --git a/internal/handler/api/v1/agent_handler.go b/internal/handler/api/v1/agent_handler.go index 29af3fa1..a733e7e4 100644 --- a/internal/handler/api/v1/agent_handler.go +++ b/internal/handler/api/v1/agent_handler.go @@ -149,6 +149,13 @@ func (h *AgentHandler) Update(c *gin.Context) { agent, svcErr := h.svc.Update(c.Request.Context(), uint(id), accountID, req) if svcErr != nil { + if errors.Is(svcErr, service.ErrAgentNameBlank) { + c.JSON(http.StatusUnprocessableEntity, gin.H{ + "message": "Name can't be blank", + "attributes": []string{"name"}, + }) + return + } applogger.L().Errorf("Update agent %d for account %d: %v", id, accountID, svcErr) handleServiceError(c, svcErr) return diff --git a/internal/handler/api/v1/agent_handler_test.go b/internal/handler/api/v1/agent_handler_test.go index 41d96482..6933f424 100644 --- a/internal/handler/api/v1/agent_handler_test.go +++ b/internal/handler/api/v1/agent_handler_test.go @@ -293,6 +293,39 @@ func (s *AgentHandlerTestSuite) TestUpdateAgent() { assert.Equal(s.T(), false, disableData["auto_offline"]) } +func (s *AgentHandlerTestSuite) TestUpdateAgentBlankNameReturnsRecordInvalidShape() { + req := service.CreateAgentRequest{ + Email: "blank-update@test.com", + Name: "Blank Update", + Role: "agent", + } + w, c := s.makeRequest("POST", "/api/v1/accounts/1/agents", req, s.account.ID, s.user.ID) + s.handler.Create(c) + assert.Equal(s.T(), http.StatusOK, w.Code) + + var created map[string]interface{} + json.Unmarshal(w.Body.Bytes(), &created) + agentID := uint(created["id"].(float64)) + + updateReq := map[string]any{ + "agent": map[string]any{ + "name": "", + }, + } + w2, c2 := s.makeRequest("PATCH", fmt.Sprintf("/api/v1/accounts/1/agents/%d", agentID), updateReq, s.account.ID, s.user.ID) + s.handler.Update(c2) + + assert.Equal(s.T(), http.StatusUnprocessableEntity, w2.Code, w2.Body.String()) + var data map[string]interface{} + json.Unmarshal(w2.Body.Bytes(), &data) + assert.Equal(s.T(), "Name can't be blank", data["message"]) + assert.Equal(s.T(), []interface{}{"name"}, data["attributes"]) + + var user model.User + s.Require().NoError(s.db.First(&user, agentID).Error) + assert.Equal(s.T(), "Blank Update", user.Name) +} + func (s *AgentHandlerTestSuite) TestDeleteAgent() { // Create an agent req := service.CreateAgentRequest{ diff --git a/internal/service/agent_service.go b/internal/service/agent_service.go index 7247cb15..4de59468 100644 --- a/internal/service/agent_service.go +++ b/internal/service/agent_service.go @@ -23,6 +23,8 @@ type AgentService struct { db *gorm.DB } +var ErrAgentNameBlank = errors.New("agent name cannot be blank") + // NewAgentService creates a new Agent service. func NewAgentService(agentRepo *repository.AgentRepo, db *gorm.DB) *AgentService { return &AgentService{agentRepo: agentRepo, db: db} @@ -54,6 +56,7 @@ type UpdateAgentRequest struct { Availability string `json:"availability,omitempty" validate:"omitempty,oneof=online offline busy"` AutoOffline bool `json:"auto_offline"` CustomRoleID *uint `json:"custom_role_id,omitempty"` + nameSet bool autoOfflineSet bool customRoleSet bool } @@ -69,11 +72,13 @@ func (r *UpdateAgentRequest) UnmarshalJSON(data []byte) error { return err } *r = UpdateAgentRequest(decoded) + _, r.nameSet = raw["name"] _, r.autoOfflineSet = raw["auto_offline"] _, r.customRoleSet = raw["custom_role_id"] return nil } +func (r UpdateAgentRequest) NameSet() bool { return r.nameSet } func (r UpdateAgentRequest) AutoOfflineSet() bool { return r.autoOfflineSet } func (r UpdateAgentRequest) CustomRoleIDSet() bool { return r.customRoleSet } @@ -134,6 +139,9 @@ func (s *AgentService) Update(ctx context.Context, userID, accountID uint, req U if err := pkgvalidator.ValidateStruct(req); err != nil { return nil, fmt.Errorf("validation: %w", err) } + if req.NameSet() && strings.TrimSpace(req.Name) == "" { + return nil, ErrAgentNameBlank + } return s.agentRepo.UpdateAgent(ctx, userID, accountID, req.Name, req.Role, req.Availability, req.AutoOffline, req.AutoOfflineSet(), req.CustomRoleID, req.CustomRoleIDSet()) }