Repository navigation
Send the partner's mapping list ranges when creating an account template - #274
michieldegezelle wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)Run `npm run lint && npm test` after code changes.📄 CodeRabbit inference engine (CLAUDE.md) Files:
🔇 Additional comments (5)
WalkthroughChangesAccount template creation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The account-template creation change is ready to merge; the tested filtering and failed-response paths have no remaining concrete issue. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
`newAccountTemplate` hardcoded `range.type === "firm"` when filtering `mapping_list_ranges` down to the target environment, so `create-account-template -p <partner-id>` always sent an empty array. A template created on a partner therefore arrived with only its base `account_range` and no mapping list association at all. The follow-up `update-account-template` then had to *add* the range rather than update it, which the API rejects with `422 Not allowed to add range for mapping list <id>`. Templates that already existed on the partner were unaffected, since their range was an update. Both sibling functions (`publishAccountTemplateByName`, `publishAccountTemplateById`) already filter on `type`; this brings the create path in line. Also hardens the same function, since a partner create can now realistically be rejected by the API: - Guard the filter with `?.` / `|| []`, matching the two update paths. A config.json without a `mapping_list_ranges` key threw a TypeError. - Check `response.status` before reading `response.data`. `sfApi` returns undefined for a 400/404, so a failed create crashed with "Cannot read properties of undefined" instead of surfacing the API error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eb24deb to
4013c56
Compare
Problem
silverfin create-account-template -p <partner-id>never sent the partner'smapping_list_ranges.newAccountTemplatefiltered them with a hardcodedrange.type === "firm":On a partner that predicate matches nothing, so the create payload always carried
mapping_list_ranges: []. The template arrived on the partner with only its baseaccount_rangeand no mapping list association.The follow-up
update-account-templatethen had to add the range rather than update it, which the API rejects:Templates that already existed on the partner were never affected — their range already exists, so the update is an update. That's the create/update split reported from the field.
Both sibling functions,
publishAccountTemplateByNameandpublishAccountTemplateById, already filter ontype. This brings the create path in line. The hardcoded"firm"dates back to #158 (Support create commands) and was never covered by a test — every existingnewAccountTemplatetest passes"firm"with empty ranges.Changes
index.js,newAccountTemplate:range.type === "firm"→range.type === type— the fix.?./|| [], matching the two update paths. Aconfig.jsonwithout amapping_list_rangeskey threw aTypeError.response.statusbefore readingresponse.data.sfApireturnsundefinedfor a 400/404 (apiUtils.responseErrorHandlerbare-returns on both), so a failed create died withCannot read properties of undefinedinstead of surfacing the API error.(2) and (3) are pre-existing, but a partner create can now realistically be rejected by the API — and (3) in particular would have masked that rejection behind an opaque TypeError, which is exactly the diagnosis this change is meant to enable.
Tests
Four new tests in
tests/lib/toolkit.test.js, each written failing first:mapping_list_rangeskey sends[]rather than throwing621/621 pass, ESLint clean.
Note on scope
This makes the CLI send the partner ranges at create time; it does not prove the API accepts them there. If a partner create still returns a 422, the remaining question is genuinely server-side — whether marketplace package assignment is a prerequisite for associating a new partner template with a mapping list. Change (3) means that answer will now be legible instead of a TypeError.
Deliberately left out, all pre-existing and none specific to the partner path: the
env_idstrict string-vs-number comparison (affects all three call sites identically),newAllAccountTemplates' lack of error containment (unreachable for partners —checkPartnerSupportrejects--allwith-p), extracting the now-thrice-duplicated filter intoAccountTemplate, and the missingversion_significant_changeon partner create.🤖 Generated with Claude Code