[CDX-262] Node SDK - Add support for item groups v2 endpoints

[TarekAlQaddy]

Adds support for our item groups V2 endpoints and deprecates V1 methods.
PR Notes:

• This change will be released in a new minor version while keeping the V1 methods functional, but marked as deprecated with a deprecation annotation and a console warning.
• The V1 methods will be removed in the next major version, along with a changelog entry and an upgrade guide.
• Since V2 endpoints do not manipulate item groups immediately and instead create a backend task that is queued and processed asynchronously, I changed the testing approach for V2:
  • Tests validate the request parameters and payload sent to the API.
  • Tests verify that a task ID is returned in the response.
  • Tests do not assert that the backend resource was created/updated, as task completion timing is asynchronous and unpredictable.
  • Since the new Delete endpoint accepts specific group IDs as parameters (rather than deleting all groups), the tests now store the IDs created during execution and clean them up in an after hook

[Copilot]

Pull request overview

Updates the Node SDK catalog item groups support to use the v2 API while keeping v1 methods available but deprecated, aligning with the async task-based behavior of the v2 endpoints.

Changes:

• Mark v1 item group methods as deprecated and emit runtime deprecation warnings.
• Add v2 item group methods: retrieve (single/list), create-or-replace, update, and delete (task-based responses).
• Introduce a new v2-focused test suite and relabel existing item group tests as “V1”.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
src/modules/catalog.js	Adds v2 item groups methods and deprecates v1 item group methods with warnings.
spec/src/modules/catalog/catalog-groups.js	Renames existing groups tests to explicitly cover v1 behavior.
spec/src/modules/catalog/catalog-groups-v2.js	Adds tests for v2 item group endpoints and task-based responses.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The deprecation strategy is well-executed — each V1 method gets both a JSDoc @deprecated annotation and a runtime console.warn, and all four V2 mutation methods include thorough JSDoc with parameters, return types, and examples.

🚨 Critical Issues

• [File: src/modules/catalog.js Line: 1444, 1518, 1585] toSnakeCaseKeys(itemGroup, false) is called with toRecurse = false, which means nested objects are not recursively converted. Specifically, the data field (e.g., { imageUrl: '...' }) and array fields like parentIds will have their keys left in camelCase. parentIds becomes parent_ids at the top level (because it's a key), which is correct — but any keys nested inside the data object (e.g., imageUrl) will not be converted to image_url. This is inconsistent with the test at line ~364 that asserts requestedBody.item_groups[0].data has property imageUrl (camelCase), suggesting the API may accept camelCase in data, but this should be explicitly documented and verified against the API spec. If the API expects image_url inside data, this is a bug; if it accepts camelCase, a comment clarifying this intent would prevent future confusion.

• [File: src/modules/catalog.js Line: 1360] retrieveItemGroup passes false as the 5th argument to createCatalogUrl (includeCParam = false), which omits the c (client version) query parameter from the URL. All other new V2 methods (and all existing V2 methods like retrieveItems) include the c param by default (includeCParam = true). The test at line 218 also does not assert requestedUrlParams.c, which masks this inconsistency. Either false is intentional (document why), or it is a bug and should be removed.

⚠️ Important Issues

• [File: spec/src/modules/catalog/catalog-groups-v2.js Line: 115, 132, 150, 306, 332, 351, 371, 388, 489, 511, 527, 550, 632, 655, 674, 689] All callback-style done tests use .then(...) without a .catch(done) or .catch((err) => done(err)). If the promise rejects (e.g., due to a network error or unexpected assertion failure), the test will hang until timeout rather than fail immediately. Add .catch(done) to every such chain.

• [File: spec/src/modules/catalog/catalog-groups-v2.js Line: 222–229] The .catch block in retrieveItemGroup's first test has a logic flaw: if error.status === 404 it calls done() and then falls through to throw error, which will re-throw after calling done() — potentially causing a "done called multiple times" error. Add a return or else before throw error:

  .catch((error) => {
    if (error.status === 404 && error.message.toLowerCase().includes('item group not found')) {
      done();
      return; // prevent fall-through to throw
    }
    throw error;
  });

• [File: src/modules/catalog.js Line: 1412, 1486, 1553] The three new async methods (createOrReplaceItemGroups, updateItemGroups, deleteItemGroups) do not wrap the await fetch(...) call in a try/catch, unlike the existing pattern in createOrReplaceItems (lines 247–267) which wraps the await in try { ... } catch (error) { return Promise.reject(error); }. An unhandled network error (e.g., abort/timeout) thrown by fetch will reject the async function's implicit promise, but it diverges from the established pattern used by the analogous createOrReplaceItems, updateItems, and deleteItems methods. The new methods should wrap the await fetch(...) call in a try/catch for consistency.

💡 Suggestions

• [File: spec/src/modules/catalog/catalog-groups-v2.js Line: 474–479] In the updateItemGroups before hook, if createOrReplaceItemGroups rejects (e.g., API unavailable), done() is never called and the entire suite hangs. Add an error handler: .then(() => { addToCleanup([mockItemGroup]); done(); }).catch(done).

• [File: src/modules/catalog.js Line: 1296–1298] The guard if (numResultsPerPage) will silently ignore numResultsPerPage: 0, which while not a valid value for this parameter, is inconsistent with how page and offset are handled (using explicit Number.isInteger checks). Consider using if (numResultsPerPage != null) or adding an explicit positive integer check for consistency.

• [File: src/modules/catalog.js Line: 1304] The offset validation offset > 0 would disallow offset: 0 (the first page of a paginated result). While 0 is functionally a no-op, the API may accept it. Consider offset >= 0 or simply checking offset != null && Number.isInteger(offset) to avoid silently dropping a valid caller-supplied value.

• [File: spec/src/modules/catalog/catalog-groups-v2.js] There is no test asserting that retrieveItemGroups correctly passes offset as a query parameter, even though offset is documented and handled in the implementation. Adding a test for it would bring parity with the numResultsPerPage and page tests.

Overall Assessment: ⚠️ Needs Work