[tudorpavel]

Adds support for the overrides and historical values API to the Node client, targeting the current API where write responses always include overrides.

• The library passes request bodies and responses through untouched, so overrides already works on Customer.create/modify, Contact.create/modify, Customer.createContact and the CustomAttribute helpers, and Customer.retrieve / Contact.retrieve already accept a trailing query object for with_overrides and attributes_with_history. Tests now pin all of that: nock body matching on every write, query serialization on retrieves, and assertions that overrides, historical_values (including entry fields) and the DELETE message survive in responses.
• Customer.attributes(config, customerUuid) hardcoded an empty query; it gains an optional params argument (Customer.attributes(config, customerUuid, { with_overrides: true })), backward compatible with a callback in third position.
• README examples added for the new params and override shapes per endpoint family, with notes on flag semantics (false removes an override and restores the most recent edit-history value; remove keeps overridden attributes and reports them in message), the shape difference between the customer and custom attribute endpoints (including the by-email response asymmetry), that attributes_with_history must be a comma-separated string rather than an array, and that contact custom attribute overrides are not yet implemented by the API.
• Tests: 296 passing (+16), lint clean; mock replies use the API's real status codes (201 on creates, 202 on deletes) and realistic values (update_performed_by is a user name, "API" or a connector name — never an email). New cases cover the params-plus-callback form of Customer.attributes, the 422 from contact custom attribute overrides with the error body callers actually get, and the 202 no-op where remove keeps an overridden attribute.

[keith-chartmogul]

LGTM, aside from a slight misalignment in the tests, which I noted with an inline comment. It would be great to address that if you can, but not the end of the world if you choose to ignore. 👍

[keith-chartmogul]

I found some more opportunities for improvement, specifically in the tests and documentation, so not strictly needed but I figure it is worth improving things as much as we can right now ahead of the release. 💪

[keith-chartmogul]

Small fixture accuracy point: the API never returns an email in update_performed_by. EditHistories::EntryBuilder#performed_by returns the user's name, "API" for API-key writes, or the data source name for external updates. Could we change this (and the one in test/chartmogul/contact.js:330) to something like 'API' or a sample name? The tests still pass either way, but these fixtures double as documentation of the response shape.

[tudorpavel]

Fixed 3711058

[keith-chartmogul]

These comments also apply to lines 174 and 374 in this file

Worth a short note here that attributes_with_history has to be a comma-separated string. Passing an array (which feels natural in JS) silently returns the wrong result:

ChartMogul.Customer.attributes(config, uuid, { attributes_with_history: ['custom.a', 'custom.b'] })
// GET .../attributes?attributes_with_history=custom.a&attributes_with_history=custom.b

Rack keeps only the last value, so the caller gets history for custom.b and nothing for custom.a, with no error. It's also capped at 100 names; more returns a 422. Something like:

// attributes_with_history: comma-separated attribute names (max 100), e.g. 'company,custom.channel'

[tudorpavel]

Added 2445b1a minus cap which isn't public and the example is already there in the code snippet. 👌

[keith-chartmogul]

Could we add a line of prose about the overrides shape? It differs between endpoint families, and the examples alone make that easy to miss:

• Customer.create / Customer.modify: custom attributes sit under attributes, e.g. { company: true, attributes: { custom: { channel: true } } }
• CustomAttribute.add / update / remove (and the Customer.attributes response): custom sits directly under overrides, e.g. { custom: { channel: true } }
• CustomAttribute.add by email: the payload uses the direct shape, but each entries[].overrides in the response uses the customer shape with the attributes wrapper

The by-email variant with overrides is covered in test/chartmogul/custom-attribute.js:102 but has no README example. Worth adding one next to this?

[tudorpavel]

Hopefully good enough dedbf6c

[keith-chartmogul]

Two behaviors that SDK users will likely hit and that aren't documented yet:

1. false unpins. On an update or remove, false releases the pin and restores the latest edit-history value. On a create it's a no-op.
2. Deleting a pinned custom attribute keeps it unless the same call unpins it. The request still returns 202, but message becomes Custom attributes deleted from customer, except overridden: X or Custom attributes are overridden and were not deleted from customer: X.

An unpin-and-remove example would cover both:

// Pinned attributes are kept on remove unless unpinned in the same call; check `message`
ChartMogul.CustomAttribute.remove(config, customerUuid, {
  'custom': ['channel'],
  'overrides': { 'custom': { 'channel': false } }
});

[tudorpavel]

Added 13c17d0

[keith-chartmogul]

Could we note that contacts only accept overrides for standard fields? A contact custom attribute in overrides is rejected with a 422 (is not an overridable attribute), because Contacts::OverridesService has no custom attribute pins. Someone extrapolating from the customer examples would probably try overrides: { custom: { ... } } here.

[tudorpavel]

Added bc98fa8

[keith-chartmogul]

Two small things on the callback test:

• The four-argument form, Customer.attributes(config, uuid, params, callback), isn't covered. Only the three-argument callback form and the promise form are. A case with both params and a callback would protect the new signature from both directions.
• Nit: AGENTS.md asks for tests that return promises rather than using done. This one could be:

it('should retrieve customer attributes with a callback as the third argument', async () => {
  // ...nock setup
  const res = await new Promise((resolve, reject) => {
    Customer.attributes(config, customerUuid, (err, body) => (err ? reject(err) : resolve(body)));
  });
  expect(res).to.have.property('tags');
});

[tudorpavel]

Looks reasonable to me 57011cd

[keith-chartmogul]

Could we add one error-path test for overrides? The most likely real-world failure is a 422 from override validation, such as pinning a contact custom attribute or sending a non-boolean flag. A test would confirm that callers get a ResourceInvalidError with the API's details on err.response:

it('rejects overrides on contact custom attributes', async () => {
  const contactUuid = 'con_00000000-0000-0000-0000-000000000000';
  /* eslint-disable camelcase */
  const patchBody = { overrides: { custom: { channel: true } } };
  const errorBody = {
    errors: { 'overrides[custom]': 'is not an overridable attribute' },
    error_details: [{ param: 'overrides[custom]', code: 'not_overridable', message: 'is not an overridable attribute' }]
  };
  /* eslint-enable camelcase */

  nock(config.API_BASE)
    .patch(`/v1/contacts/${contactUuid}`, patchBody)
    .reply(422, errorBody);

  const err = await Contact.modify(config, contactUuid, patchBody).then(
    () => { throw new Error('expected rejection'); },
    e => e
  );
  expect(err).to.be.instanceOf(ChartMogul.ResourceInvalidError);
  expect(err.response).to.eql(errorBody);
});

(The explicit throw matters: the existing .catch-only pattern around customer.js:95 passes even when the promise resolves.)

[tudorpavel]

Added 354eed6, but Claude friend found some more general issue about superagent?

Added the test with your explicit-throw guard — but with the assertions adapted, because the suggested ones exposed something pre-existing: ResourceInvalidError (and the whole typed 4xx mapping in resource.js) has been unreachable since the superagent migration. Superagent errors on any 4xx and resource.js rejects that error before the status switch runs — the existing "should throw NotFoundError/ResourceInvalidError" tests in test/chartmogul/resource.js actually assert superagent's error shape (e.status, e.response), not the typed classes. So the SDK's de facto error contract today is superagent errors, and the new test pins that: err.status === 422 and err.response.body carrying the API's errors/error_details. Restoring the typed mapping would change the rejected error's shape for every consumer, which is a breaking change out of scope for this additive PR — I'd suggest a separate issue for it.

[keith-chartmogul]

Optional: a second remove case where the attribute stays pinned (no false override) and the 202 reply carries message: 'Custom attributes are overridden and were not deleted from customer: CAC' would record that the 202 can hide a no-op. That's probably the most surprising behavior for SDK users.

[tudorpavel]

Added 3e0d5c2