Fix confirmations and notifications validation to accept objects

[Mwalek]

Summary

• Fix confirmations and notifications validation to accept objects (keyed by ID) instead of arrays
• Add missing notifications validation in both legacy and new validator paths

Problem

The Gravity Forms REST API v2 returns confirmations and notifications as objects keyed by ID, not arrays. However, both validators incorrectly enforced array validation:

• validators.js: Used .array() on the confirmations field
• validation.js: Called validateArray() on confirmations

This caused all gf_create_form and gf_update_form calls with confirmations or notifications to fail with:

Validation error for gf_create_form: confirmations must be an array

Additionally, notifications had no validation at all in either validator, meaning they were silently stripped from the validated output.

Steps to Reproduce

1. Try to create or update a form with confirmations:

   {
     "title": "Test Form",
     "confirmations": {
       "abc123": {
         "id": "abc123",
         "name": "Default Confirmation",
         "type": "message",
         "message": "Thanks!"
       }
     }
   }

2. Expected: Form created/updated with confirmation
3. Actual: Validation error: confirmations must be an array

Solution

• Changed validateArray() → validateObject() for confirmations in validation.js
• Changed .array() → .object() for confirmations in validators.js
• Updated iteration from array methods (.map(), .forEach()) to Object.entries()
• Added notifications validation as .object() in both validators

Test plan

• Verified gf_update_form with confirmations object succeeds (previously failed)
• Verified gf_update_form with notifications object succeeds (previously stripped)
• Confirmed form data round-trips correctly (create → get → update → get)

Summary by CodeRabbit

• New Features

  • Added support for keyed notifications in form validation/schema.

• Bug Fixes

  • Improved API access validation and request signing with more reliable URL handling.
  • Refined confirmations validation and error path reporting to use keyed entries instead of array indices.

• Tests

  • Updated test fixtures and integration/validation tests to match the new confirmations/notifications data shape.

[coderabbitai]

Walkthrough

Refactors confirmations/notifications from arrays to keyed objects across validation, schema, tests, and test data; updates OAuth signing to build and validate full endpoint URLs from httpClient.defaults.baseURL; adds defaults.baseURL to MockHttpClient for tests.

Changes

Cohort / File(s)	Summary
Validation/schema updates src/config/validators.js, src/config/validation.js	Change schema: confirmations (and new notifications) are now objects rather than arrays. Validation iterates via Object.entries, and error paths use dot-notation (confirmations.{key}.url) instead of indexed paths.
Authentication URL signing src/config/auth.js	Require and validate httpClient.defaults.baseURL; construct fullUrl = baseURL + endpoint.path and generate OAuth-signed headers per-endpoint using the full URL before pinging endpoints.
Test data and tests scripts/setup-test-data.js, src/tests/integration.test.js, src/tests/validation.test.js	Test fixtures and tests converted confirmations/notifications from arrays to keyed objects (e.g., conf_1, notif_1); tests updated to submit and validate the new object-shaped formData.
Test helper src/tests/helpers.js	MockHttpClient gains a defaults property with baseURL: 'https://test.example.com' to support auth validation that reads httpClient.defaults.baseURL.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant AuthModule as Auth
participant HttpClient as HTTP Client
participant Endpoint as External Endpoint

Client->>Auth: validateRestApiAccess(httpClient, endpoints)
Auth->>HttpClient: read httpClient.defaults.baseURL
alt baseURL missing
    Auth-->>Client: throw Error("missing baseURL")
else
    loop for each endpoint
        Auth->>Auth: fullUrl = baseURL + endpoint.path
        Auth->>Auth: headers = getAuthHeaders('GET', fullUrl, { per_page: 1 })
        Auth->>HttpClient: GET fullUrl with headers
        HttpClient->>Endpoint: request fullUrl
        Endpoint-->>HttpClient: response
        HttpClient-->>Auth: response
    end
    Auth-->>Client: aggregated endpoints status
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: converting confirmations and notifications validation from arrays to objects to match the Gravity Forms REST API v2 response format.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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)

src/config/auth.js (1)

327-345: ⚠️ Potential issue | 🟡 Minor

baseURL requirement applies universally even for Basic Auth.

BasicAuthHandler.getAuthHeaders() accepts no arguments and ignores fullUrl, so the mandatory baseURL check at lines 323–325 adds a new runtime requirement for validateRestApiAccess even in Basic Auth deployments. A custom httpClient that lacks defaults.baseURL would cause this health-check to fail. Consider short-circuiting the check when the active auth handler is BasicAuthHandler.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/auth.js` around lines 327 - 345, The health-check adds a mandatory
baseURL requirement even for BasicAuth deployments; update validateRestApiAccess
to detect when the active auth handler is BasicAuthHandler (e.g., via
authManager.activeHandler or checking authManager.getAuthHeaders
implementation/constructor name) and skip the baseURL existence check in that
case, and when calling authManager.getAuthHeaders use the signature appropriate
for the handler (call getAuthHeaders('GET', fullUrl, ...) only for handlers that
accept a full URL, otherwise call BasicAuthHandler.getAuthHeaders() with no
args), keeping the httpClient.get call using endpoint.path and not assuming
httpClient.defaults.baseURL exists.

🧹 Nitpick comments (1)

src/config/validators.js (1)

90-106: !Array.isArray(confirmations) guard is redundant after .object().

The .object() chain rule already rejects arrays before the custom callback runs, making this condition unreachable. The confirmations && null/undefined guard is still needed, but the array check can be dropped.

♻️ Proposed simplification

 .custom((confirmations) => {
-  if (confirmations && typeof confirmations === 'object' && !Array.isArray(confirmations)) {
+  if (confirmations && typeof confirmations === 'object') {
     Object.entries(confirmations).forEach(([key, conf]) => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/validators.js` around lines 90 - 106, In the custom validator
attached to field('confirmations') (the
validate('confirmations').object().custom(...) block), remove the redundant
Array.isArray(confirmations) check since .object() already rejects arrays; keep
the null/undefined guard (confirmations && typeof confirmations === 'object') so
the custom logic still skips when confirmations is missing, and otherwise
iterate Object.entries(confirmations) to validate redirect entries as before
(i.e., call
validate(`confirmations.${key}.url`).required().string().url().validate(conf.url)
when conf.type === 'redirect' and conf.url !== undefined).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/config/auth.js`:
- Around line 320-325: The code reads httpClient.defaults.baseURL directly which
can throw if httpClient.defaults is undefined and get swallowed by the outer
catch; change the lookup to use optional chaining (e.g., access
httpClient?.defaults?.baseURL) and keep the existing explicit guard that throws
'httpClient baseURL is not configured' when baseURL is falsy so the intended
error message is produced; update the reference in the same block where baseURL
is declared (the httpClient.defaults/baseURL access) to use optional chaining.

In `@src/config/validation.js`:
- Around line 311-317: validateObject currently rejects arrays, but
formData.confirmations is treated as an array in multiple tests and scripts;
update all places that set confirmations to use the keyed-object Gravity Forms
v2 shape instead of arrays. Replace array usage in
src/tests/integration.test.js, src/tests/validation.test.js, and
scripts/setup-test-data.js so confirmations are objects keyed like
"confirmation_1": { type: ..., url: ... } (so
validateObject(formData.confirmations, 'confirmations') and the subsequent loop
that calls validateURL(conf.url, `confirmations.${key}.url`) will work without
errors).

---

Outside diff comments:
In `@src/config/auth.js`:
- Around line 327-345: The health-check adds a mandatory baseURL requirement
even for BasicAuth deployments; update validateRestApiAccess to detect when the
active auth handler is BasicAuthHandler (e.g., via authManager.activeHandler or
checking authManager.getAuthHeaders implementation/constructor name) and skip
the baseURL existence check in that case, and when calling
authManager.getAuthHeaders use the signature appropriate for the handler (call
getAuthHeaders('GET', fullUrl, ...) only for handlers that accept a full URL,
otherwise call BasicAuthHandler.getAuthHeaders() with no args), keeping the
httpClient.get call using endpoint.path and not assuming
httpClient.defaults.baseURL exists.

---

Nitpick comments:
In `@src/config/validators.js`:
- Around line 90-106: In the custom validator attached to field('confirmations')
(the validate('confirmations').object().custom(...) block), remove the redundant
Array.isArray(confirmations) check since .object() already rejects arrays; keep
the null/undefined guard (confirmations && typeof confirmations === 'object') so
the custom logic still skips when confirmations is missing, and otherwise
iterate Object.entries(confirmations) to validate redirect entries as before
(i.e., call
validate(`confirmations.${key}.url`).required().string().url().validate(conf.url)
when conf.type === 'redirect' and conf.url !== undefined).

[coderabbitai]

🧹 Nitpick comments (1)

src/tests/validation.test.js (1)

240-258: Consider adding a type validation test for notifications.

The existing "Type Validation: Objects" tests cover search and meta, but there's no explicit test asserting that notifications rejects non-object values (e.g., passing a string or array). Since notifications is a newly added validated field, a quick test would solidify coverage.

💡 Suggested test addition (outside of current line range)

// Notifications should be object
await TestAssert.throwsAsync(
  () => client.createForm({
    title: 'Test',
    notifications: 'not-an-object'
  }),
  'must be an object',
  'Notifications parameter should be object'
);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tests/validation.test.js` around lines 240 - 258, Add a type validation
test asserting that the new notifications field rejects non-object values: in
the existing "Type Validation: Objects" suite add a TestAssert.throwsAsync call
that invokes client.createForm with notifications set to a non-object (e.g., a
string or array) and expects the error message 'must be an object' and a
descriptive assertion message like 'Notifications parameter should be object';
reference the client.createForm call and the notifications field when locating
where to insert the test.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/tests/validation.test.js`:
- Around line 240-258: Add a type validation test asserting that the new
notifications field rejects non-object values: in the existing "Type Validation:
Objects" suite add a TestAssert.throwsAsync call that invokes client.createForm
with notifications set to a non-object (e.g., a string or array) and expects the
error message 'must be an object' and a descriptive assertion message like
'Notifications parameter should be object'; reference the client.createForm call
and the notifications field when locating where to insert the test.

[zackkatz]

@claude Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @src/tests/validation.test.js:

• Around line 240-258: Add a type validation test asserting that the new
  notifications field rejects non-object values: in the existing "Type Validation:
  Objects" suite add a TestAssert.throwsAsync call that invokes client.createForm
  with notifications set to a non-object (e.g., a string or array) and expects the
  error message 'must be an object' and a descriptive assertion message like
  'Notifications parameter should be object'; reference the client.createForm call
  and the notifications field when locating where to insert the test.