Skip to content

Add company-data-copier command#264

Open
BenjaminLangenakenSF wants to merge 6 commits into
mainfrom
add-company-data-copier-command
Open

Add company-data-copier command#264
BenjaminLangenakenSF wants to merge 6 commits into
mainfrom
add-company-data-copier-command

Conversation

@BenjaminLangenakenSF

@BenjaminLangenakenSF BenjaminLangenakenSF commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What

Adds a new silverfin company-data-copier CLI command that triggers the platform's Data Copier to copy a source company's data into a brand-new company in a destination (development) firm. This lets BSO developers reproduce a client's situation in a dev firm to debug templates against realistic data, without ever touching the production firm.

silverfin company-data-copier -c <source_company_id> -l <period_id_1> <period_id_2> -f <destination_firm_id>
  • -c, --source-company-id — company to copy data from (in the source/client firm)
  • -l, --source-ledger-ids — one or more period ids, space-separated (variadic, matching run-test's --handle)
  • -f, --firm — destination firm whose token authenticates the request and where the copy is created (defaults to the configured default firm)

It copies data only (account values incl. adjustments, text properties, people/company drop, configuration) — not template code. The destination firm is taken from the URL/token; the source is identified purely by company + period ids in the body (no source-firm parameter). The platform enforces source-firm access rights on the authenticating user (see the access note below).

How it wires up

Follows the existing layering exactly:

  • lib/api/sfApi.jsrunCompanyDataCopier, reusing AxiosFactory.createInstance (so staging Basic-auth, access_token, and token refresh all work unchanged).
  • index.jscopyCompanyData (input validation + reporting).
  • bin/cli.js → the company-data-copier command.

✅ Route verified on live — it is the firm-scoped v4 route

An earlier revision of this command POSTed to the public, non-firm-scoped /api/public/v3/company_data_copier/run, on the basis that doc/bso_company_data_copier.md said the firm-scoped route had been renamed away (commit 24e2547bc9). Live probing shows that is not the case — the copier is served at the firm-scoped route:

Request on live.getsilverfin.com Result
POST /api/public/v3/company_data_copier/run 404 page not found — route does not exist
POST /api/v4/f/:id/company_data_copier/run 401 invalid_token — route exists, only auth missing; returns a proper 422 business-validation once authenticated

So this PR now POSTs the relative company_data_copier/run, inheriting the firm axios instance's /api/v4/f/:id baseURL (and its token, staging Basic-auth and refresh interceptor) — the "revert this one line" the previous note anticipated.

@team-reconciliation — the doc's claim that /api/v4/f/:id/company_data_copier/run was renamed away in favour of a public-v3 route does not match the deployed live behaviour. Please update doc/bso_company_data_copier.md.

Two other alignments with the merged backend (unchanged from the previous revision):

  • Copied-company name. The backend names the company Company_<source_id>_<hex> (e.g. Company_42_a3f1b9d4), not SF_COPY_<id>_<timestamp>. Success message + README reflect this.
  • Fire-and-forget, honestly. 202 {"status":"enqueued"} means enqueued, not copied — the copy runs in a Sidekiq job and there is no status endpoint. Messaging says the copy was requested, that it runs async, to verify in the destination firm after a few minutes, and that a company name ending in _FAILED means the (non-retried) copy failed.

⚠️ Testing with real client data requires a support user

The feature's purpose is reproducing client data in a dev firm, but doing so runs into an access boundary worth flagging explicitly:

The command authenticates as the destination-firm user, and the backend resolves the source company in that user's context. Reading a company that lives in a different (client) firm therefore requires the authenticating user to be either:

  • a Silverfin support user (silverfin_support?), which can read any firm's companies, or
  • a member of both the source and destination firms.

A normal BSO-developer account cannot reach client firms — the OAuth authorize page only lists the internal dev/demo/testing firms the user belongs to — so attempting a client source yields 422 {"error":"source_company_id <id> does not exist"} (the backend reports cross-firm invisibility as "does not exist"). This is almost certainly intentional given the data-protection implications of copying production client data into a dev firm.

Open question for the team: how are BSO developers meant to authenticate for real client-data copies? Options: a scoped support credential for this workflow, support-initiated copies, or a consent/approval flow. Note that copies within firms the developer already belongs to (e.g. functional-testing → demo) work today and are sufficient to validate the command itself end-to-end (404 → 401 → 422 validation → 202 enqueued).

Backend prerequisites / gotchas (now in the README)

These cause 422s or silent skips that would otherwise confuse users:

  • Destination firm must be a demo firm (trial/customer/junk/churn → 422).
  • Source firm must differ from destination firm (422).
  • All source_ledger_ids must belong to source_company_id (422).
  • Templates are matched to the destination firm by handle (reconciliations) / Dutch name name_nl (account detail templates). If the template doesn't already exist in the destination firm, its data is silently skipped — so the template you're debugging must already exist there.
  • Caller must be a Silverfin support user or have access to the source firm (see access note above).
  • One copy per destination firm at a time (Redis mutex, 5-min TTL). A second run fired at the same destination while one is in flight silently does nothing — don't fire the command repeatedly.

Also included

  • Removed a misleading suffix from the shared 422 error handler (apiUtils.js) — a 422 isn't specifically a permissions issue, so "You don't have the rights to update the previous parameters" was confusing (it showed on a Data Copier "source_company_id does not exist" error). Now only the real API error is surfaced. Applies to all commands.
  • README section + CHANGELOG entry, version bump to 1.57.0.

Testing

  • sfApi HTTP layer (axios-mock-adapter) asserts the request hits the firm-scoped company_data_copier/run route (relative to the firm /api/v4/f/:id baseURL).
  • E2E command test (tests/bin/cli/) asserts the success message reports requested/enqueued and the follow-up guidance flags the async nature + _FAILED signal.
  • Commander wiring in tests/bin/cli.test.js, plus TESTS.md catalogue entries.
  • Full suite green, lint clean.

🤖 Generated with Claude Code

BenjaminLangenakenSF and others added 4 commits July 9, 2026 17:25
Trigger the platform Data Copier to copy a source company's data into a
brand-new company in a destination (development) firm, so BSO developers
can reproduce a client's situation in a dev firm without touching the
production firm.

- sfApi.runCompanyDataCopier: POST /api/v4/f/:firm_id/company_data_copier/run
- index.copyCompanyData: validates input and reports the enqueued job
- bin/cli.js: 'company-data-copier' command (-c source company, -l source
  period ids (variadic), -f destination firm)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A 422 is not specifically a permissions problem, so the hardcoded "You
don't have the rights to update the previous parameters" line was
misleading (e.g. it appeared on a Data Copier "source_company_id does
not exist" error). Surface only the real API error message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers the sfApi HTTP call (axios-mock-adapter), the E2E command test in
tests/bin/cli/, and Commander wiring in tests/bin/cli.test.js, plus the
TESTS.md catalogue entries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds a new company-data-copier CLI command and supporting copyCompanyData toolkit function that calls a new runCompanyDataCopier API helper to copy source company data into a new destination company. Includes documentation, changelog, version bump, tests, and updated 422 error logging.

Changes

Company Data Copier Feature

Layer / File(s) Summary
API layer: runCompanyDataCopier
lib/api/sfApi.js, lib/utils/apiUtils.js, tests/lib/api/sfApi.test.js
Adds the API request helper for the firm-scoped company_data_copier/run endpoint, exports it, tests success and 422 handling, and logs serialized 422 response data.
Toolkit copyCompanyData implementation
index.js, tests/bin/cli/company-data-copier.test.js
Adds input validation, API invocation, result logging, public export, and tests for success, missing inputs, and missing response data.
CLI command wiring and validation
bin/cli.js, tests/bin/cli.test.js
Adds the company-data-copier subcommand with --source-company-id, --source-ledger-ids, and --firm options, numeric validation, and help output tests.
Documentation, changelog, and version bump
README.md, CHANGELOG.md, package.json, tests/TESTS.md
Documents command usage and prerequisites, adds the 1.57.0 changelog entry, updates the package version, and records the related test coverage.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and directly names the main change: adding the company-data-copier command.
Description check ✅ Passed The description covers the change and testing well, though it omits some template sections like Fixes #, step-by-step instructions, and checklists.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-company-data-copier-command

Comment @coderabbitai help to get the list of available commands.

@BenjaminLangenakenSF BenjaminLangenakenSF self-assigned this Jul 9, 2026
@BenjaminLangenakenSF BenjaminLangenakenSF added the feature request New feature or request label Jul 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
tests/bin/cli/company-data-copier.test.js (2)

77-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test only covers undefined response, not a "truthy response without .data" case.

The guard in copyCompanyData is !response || !response.data; only the !response branch is exercised here. Consider adding a case like SF.runCompanyDataCopier.mockResolvedValue({}) to also cover the "response exists but has no data" path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/bin/cli/company-data-copier.test.js` around lines 77 - 84, The current
test for copyCompanyData only covers the undefined response path, but not the
case where SF.runCompanyDataCopier returns an object without data. Update the
company-data-copier test suite to add a separate case that mock-resolves a
truthy response with no data (for example via the copyCompanyData flow) and
asserts the same error/false behavior, so both branches of the !response ||
!response.data guard are exercised.

21-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused e2e-style scaffolding in a pure unit test suite.

tempDir/chdir/process.exit mocking (lines 24-30, 41-43) are never exercised — copyCompanyData doesn't touch the filesystem or call process.exit. This looks copy-pasted from an e2e CLI test file and just adds noise.

♻️ Suggested simplification
-const fsPromises = require("fs").promises;
-const path = require("path");
-const os = require("os");
-
 jest.mock("consola");
 jest.mock("../../../lib/api/sfApi");

 const SF = require("../../../lib/api/sfApi");
 const consola = require("consola");
 const toolkit = require("../../../index");

 describe("company-data-copier", () => {
-  let tempDir;
-  let originalCwd;
-  let originalExit;
-
   const destinationFirmId = 13692;
   const sourceCompanyId = 1224550;
   const sourceLedgerIds = [33417839, 32116688];

   beforeEach(async () => {
     jest.clearAllMocks();
-
-    tempDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), "sf-cli-test-"));
-
-    originalCwd = process.cwd();
-    process.chdir(tempDir);
-
-    originalExit = process.exit;
-    process.exit = jest.fn();

     consola.success = jest.fn();
     consola.error = jest.fn();
     consola.info = jest.fn();
     consola.log = jest.fn();
     consola.warn = jest.fn();
     consola.debug = jest.fn();
   });
-
-  afterEach(async () => {
-    process.chdir(originalCwd);
-    process.exit = originalExit;
-    await fsPromises.rm(tempDir, { recursive: true, force: true });
-  });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/bin/cli/company-data-copier.test.js` around lines 21 - 44, The test
setup in company-data-copier.test.js includes unused e2e-style scaffolding that
`copyCompanyData` never exercises. Remove the unnecessary `tempDir` creation,
`process.chdir`/`originalCwd` handling, and `process.exit` mocking from
`beforeEach`/`afterEach`, and keep only the mocks actually needed for the unit
tests such as the `consola` methods and `jest.clearAllMocks()`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/bin/cli/company-data-copier.test.js`:
- Around line 77-84: The current test for copyCompanyData only covers the
undefined response path, but not the case where SF.runCompanyDataCopier returns
an object without data. Update the company-data-copier test suite to add a
separate case that mock-resolves a truthy response with no data (for example via
the copyCompanyData flow) and asserts the same error/false behavior, so both
branches of the !response || !response.data guard are exercised.
- Around line 21-44: The test setup in company-data-copier.test.js includes
unused e2e-style scaffolding that `copyCompanyData` never exercises. Remove the
unnecessary `tempDir` creation, `process.chdir`/`originalCwd` handling, and
`process.exit` mocking from `beforeEach`/`afterEach`, and keep only the mocks
actually needed for the unit tests such as the `consola` methods and
`jest.clearAllMocks()`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c6576f3c-3c1f-4198-8542-fb631ebcb724

📥 Commits

Reviewing files that changed from the base of the PR and between 258e6b7 and 2f72045.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (11)
  • CHANGELOG.md
  • README.md
  • bin/cli.js
  • index.js
  • lib/api/sfApi.js
  • lib/utils/apiUtils.js
  • package.json
  • tests/TESTS.md
  • tests/bin/cli.test.js
  • tests/bin/cli/company-data-copier.test.js
  • tests/lib/api/sfApi.test.js

BenjaminLangenakenSF and others added 2 commits July 10, 2026 11:50
Based on a review of the merged backend (GitLab MRs !26606 / !26472 and
doc/bso_company_data_copier.md):

- Route: POST the public, non-firm-scoped `/api/public/v3/company_data_copier/run`
  (destination firm derived from the OAuth token) instead of the firm-scoped
  `/api/v4/f/:id/...` route, which the doc says was renamed away. Uses an
  absolute URL to bypass the firm instance's baseURL while keeping the
  destination firm's token/auth/refresh.
- Copied-company name: the backend names it `Company_<source_id>_<hex>`
  (with `_FAILED` appended on failure), not `SF_COPY_<id>_<timestamp>`.
  Updated the success message and README.
- Honest fire-and-forget messaging: `202` means enqueued, not copied. There is
  no status endpoint; verify in the destination firm after a few minutes and
  treat a `_FAILED` company name as a failed (non-retried) copy.
- README: documented backend prerequisites (demo-firm-only, source != dest,
  ledgers must belong to the source company, templates matched by handle /
  name_nl are silently skipped if absent, caller-rights) and the per-firm
  concurrency mutex (silent no-op while a copy is in flight).
- Tests updated to assert the public v3 URL and the fire-and-forget messaging.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The command POSTed to /api/public/v3/company_data_copier/run, which
returns 404 on live. Probing production confirmed the copier is served
at the firm-scoped /api/v4/f/:id/company_data_copier/run route:

  - POST /api/public/v3/company_data_copier/run  -> 404 page not found
  - POST /api/v4/f/:id/company_data_copier/run   -> 401 invalid_token
    (route exists, only auth missing; returns proper 422 once authed)

Switch runCompanyDataCopier to POST the relative "company_data_copier/run"
path so it inherits the firm axios instance's /api/v4/f/:id baseURL along
with its token, staging Basic-auth and refresh interceptor. Drop the now
unused firmCredentials import and update the sfApi test to assert the
firm-scoped route.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant