diff --git a/docs/backend/admin-controllers.md b/docs/backend/admin-controllers.md index 7a4cfd28..7d3e999c 100644 --- a/docs/backend/admin-controllers.md +++ b/docs/backend/admin-controllers.md @@ -609,6 +609,7 @@ Every field and tab of a compiled form has a stable name: the field key, and the An extension adds value fields (`text`, `textarea`, `number`, `checkbox`, `switch`, `dropdown` with listed options, `markdown`, `mltext`, `mltextarea`, `mlmarkdown` and plugin field types), `widget` fields and `partial` fields. Types that bind to the owner's model are refused at boot, `fileupload` included: an extension owns no attachments, so a plugin field type with `Upload` stores files instead. - On save, the extension's keys leave the request body before anything else reads it, so they never reach `Fill`, the owner's columns, its translation rows or its attachments. After the owner row is written (and its translations and after hooks have run), `Save` runs inside the same transaction with `pact.AdminFormSave`: the `Record` with its `ID`, `Created`, `Values`, the submitted values of the extension's fields whose `context` allows the operation, and `Changes`, what the owner's save changed (old and new values, a translated value per locale), as the save's `pact.RecordSaved` will report it after the commit. `Save` runs on every create and update, with or without values. +- `required: true` on an extension field is checked on the submitted value, not as a rule on the owner's model, and only for administrators who see the extension: a create must send a value that is not null or blank (for a translatable field, the default locale's text), an update checks the value only when it is sent. - A translatable field (`mltext`, `mltextarea`, `mlmarkdown`, a translatable plugin field type) reaches `Save` as a `map[string]string` of locale to text, and `Load` returns it the same way. A plugin field type's `Decode` runs before `Save` (its notices are kept, a `pact.FormFieldError` is a 422 on the field) and its `Load` after the extension's `Load`. - A `Save` error rolls the whole save back, the owner's row included. `pact.AdminFormErrors` (field name to messages, phrase keys or text) answers 422 with those lines on the fields; any other error is logged and answered with the generic 500 body. `pact.AdminFormSaveResult.Notices` are save notices per field, shown on the field like a field type's notices; a notice may name an owner field. - On show and in the save response, `Load` returns the values by field name; keys that are not the extension's fields are dropped. diff --git a/modules/cabana/crud.go b/modules/cabana/crud.go index 593f1ab1..2dd940a6 100644 --- a/modules/cabana/crud.go +++ b/modules/cabana/crud.go @@ -1351,6 +1351,12 @@ func mergedRules(ctx context.Context, cc *CompiledController, model any, op stri return out } for _, field := range cc.Form.Fields { + // D-91: a form extension's field is not a column of this model; its + // required is checked on the lifted value (liftExtensionValues), and + // only for administrators who see the extension. + if cc.extensionOf(field.Name) != nil { + continue + } // Relation fields are not writable columns. required stays on the // schema for the client, but it cannot be checked by Fill. A virtual // field is checked against its submitted value. diff --git a/modules/cabana/form_extensions.go b/modules/cabana/form_extensions.go index fe3964ca..7ea5677b 100644 --- a/modules/cabana/form_extensions.go +++ b/modules/cabana/form_extensions.go @@ -1161,7 +1161,8 @@ type extensionLift struct { // extension keeps the values of its value fields whose context allows the // operation: a translatable one as a map of locale to text (a string is the // default locale's text), any other built-in type as a single JSON value, a -// plugin field type as sent. +// plugin field type as sent. A required field of a visible extension is +// checked here, never against the owner's model (mergedRules skips it). func liftExtensionValues(ctx context.Context, in extensionLift, body map[string]any) (extensionValues, error) { out := extensionValues{} cc := in.CC @@ -1176,14 +1177,21 @@ func liftExtensionValues(ctx context.Context, in extensionLift, body map[string] for _, ext := range cc.formExtensions { for _, name := range ext.fields { value, present := body[name] - if !present { - continue - } delete(body, name) if !in.Visible[ext] || !ext.valueField(name) || !contextAllows(cc, name, in.Op) { continue } field := ext.own[name] + // A required extension field must be sent on create; on update + // only a sent value is checked, so a partial save keeps the + // stored one. Blank is null or whitespace, for a translatable + // field the default locale's text. + if !present { + if field.Required && in.Op == "create" { + details[name] = []string{"The " + name + " field is required."} + } + continue + } if isMLField(field) { if !resolved { var err error @@ -1197,10 +1205,17 @@ func liftExtensionValues(ctx context.Context, in extensionLift, body map[string] details[name] = []string{message} continue } + if def, sent := texts[locales.def]; field.Required && ((sent && blankValue(def)) || (!sent && in.Op == "create")) { + details[name] = []string{"The " + name + " field is required."} + continue + } value = texts } else if field.fieldType == nil && nestedValue(value) { details[name] = []string{"The " + name + " field must be a single value."} continue + } else if field.Required && blankValue(value) { + details[name] = []string{"The " + name + " field is required."} + continue } if out[ext] == nil { out[ext] = map[string]any{} diff --git a/modules/cabana/form_extensions_postgres_test.go b/modules/cabana/form_extensions_postgres_test.go index fc90a74e..0285f8e9 100644 --- a/modules/cabana/form_extensions_postgres_test.go +++ b/modules/cabana/form_extensions_postgres_test.go @@ -533,3 +533,68 @@ func TestFormExtensionSave(t *testing.T) { } }) } + +// TestFormExtensionRequired (CR-01): a required extension field is checked +// on the lifted value, never as a rule on the owner's model. An administrator +// who sees the extension must send it on create and may not blank it; one who +// does not see it saves the host form freely. +func TestFormExtensionRequired(t *testing.T) { + store := &feStore{} + extra := feContributor{ + id: "acme.extra", + fsys: feExtraFS("fields:\n c:\n type: text\n required: true\n after: a\n"), + extensions: []pact.AdminActionExtension{{ + Controller: "acme.blog.posts", + Form: &pact.AdminFormExtension{Fields: "extensions/posts.yaml", Permissions: []string{"acme.extra.manage"}, Load: store.load, Save: store.save}, + }}, + } + env := newFEEnv(t, feOwner{fields: feOwnerFields}, extra) + editor := &bouncer.Principal{ID: 2, Backend: true, PermissionGrants: map[string]bool{"acme.blog.access": true}} + create := func(principal *bouncer.Principal, body map[string]any) *httptest.ResponseRecorder { + t.Helper() + return feCall(t, env.svc, principal, "acme.blog.posts", http.MethodPost, "record", "", body) + } + + for name, body := range map[string]map[string]any{ + "missing": {"a": "A"}, + "blank": {"a": "A", "c": " "}, + "null": {"a": "A", "c": nil}, + } { + rec := create(superUser(), body) + if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), "The c field is required.") { + t.Fatalf("%s: status=%d body=%s", name, rec.Code, rec.Body.String()) + } + } + rec := create(superUser(), map[string]any{"a": "A", "c": "C"}) + if rec.Code != http.StatusCreated { + t.Fatalf("create with c: status=%d body=%s", rec.Code, rec.Body.String()) + } + created := pgFTDecode(t, rec) + idText := "" + switch v := created.Data["id"].(type) { + case float64: + idText = strconv.FormatFloat(v, 'f', -1, 64) + case string: + idText = v + } + if idText == "" { + t.Fatalf("create data=%v", created.Data) + } + + // An update without c keeps the stored value; a blank c is refused. + if rec := feCall(t, env.svc, superUser(), "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "A2"}); rec.Code != http.StatusOK { + t.Fatalf("partial update: status=%d body=%s", rec.Code, rec.Body.String()) + } + if rec := feCall(t, env.svc, superUser(), "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "A3", "c": ""}); rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("blank update: status=%d body=%s", rec.Code, rec.Body.String()) + } + + // The extension is hidden from editor: required does not apply to them. + rec = create(editor, map[string]any{"a": "E"}) + if rec.Code != http.StatusCreated { + t.Fatalf("editor create: status=%d body=%s", rec.Code, rec.Body.String()) + } + if rec := feCall(t, env.svc, editor, "acme.blog.posts", http.MethodPut, "record", idText, map[string]any{"a": "E2", "c": ""}); rec.Code != http.StatusOK { + t.Fatalf("editor update: status=%d body=%s", rec.Code, rec.Body.String()) + } +}