Implement reverse test#249
Open
Benjvandam wants to merge 18 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/cli.js (1)
540-567: Consider adding URL validation and more detailed error output.Two minor observations:
If
liquidTestUtils.extractURLfails to parse an invalid URL, the error may not be user-friendly. Consider validating the parsedurlDatabefore proceeding.On upload failure (line 564), consider logging the response details to help with debugging:
🔧 Optional: Improve error messaging
// Upload to Silverfin const response = await SF.updateReconciliationCustom(firmId, companyId, periodId, reconciliationId, properties); if (response && response.status >= 200 && response.status < 300) { consola.success("Text properties updated successfully"); } else { - consola.error("Failed to update text properties"); + consola.error("Failed to update text properties", response?.data || response?.statusText || ""); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli.js` around lines 540 - 567, Wrap the call to liquidTestUtils.extractURL in a try/catch and validate the returned urlData (check that firmId, companyId, periodId and reconciliationId are present) and log a clear user-facing error and exit if parsing fails or required IDs are missing; after calling SF.updateReconciliationCustom, enhance the failure branch to log detailed response information (e.g., response.status, response.data or response.error) before calling process.exit(1) so failures are easier to debug — reference extractURL, urlData (firmId, companyId, periodId, reconciliationId), findTestData, transformCustomToProperties, SF.updateReconciliationCustom and response when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/utils/textPropertyUtils.js`:
- Around line 27-37: The code currently has inconsistent behavior when mixing
simple keys ("ns.key") and nested keys ("ns.key.sub") because namespaceMap
entries are set once and later keys either get ignored or overwrite the value;
update the logic in the block that handles keyParts and namespaceMap so
conflicts are merged deterministically: whenever adding a nested key
(keyParts.length > 2) and namespaceMap already has a primitive value for
namespaceKey, convert that entry's value into an object and store the original
primitive under a reserved property (e.g., "" or "_value"); conversely, when
adding a simple key and an entry exists whose value is an object, store the
simple value under that reserved property instead of replacing the object; use
the existing symbols namespaceMap, namespaceKey, keyParts, subKey and ensure you
consistently create/merge objects rather than overwriting or ignoring entries.
---
Nitpick comments:
In `@bin/cli.js`:
- Around line 540-567: Wrap the call to liquidTestUtils.extractURL in a
try/catch and validate the returned urlData (check that firmId, companyId,
periodId and reconciliationId are present) and log a clear user-facing error and
exit if parsing fails or required IDs are missing; after calling
SF.updateReconciliationCustom, enhance the failure branch to log detailed
response information (e.g., response.status, response.data or response.error)
before calling process.exit(1) so failures are easier to debug — reference
extractURL, urlData (firmId, companyId, periodId, reconciliationId),
findTestData, transformCustomToProperties, SF.updateReconciliationCustom and
response when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eb889f5-0776-47ba-8d67-f95f23f16839
📒 Files selected for processing (5)
.gitignoreCHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/utils/textPropertyUtils.js
Extract and push custom data from all 4 levels in liquid test YAML: company, period, reconciliation, and account. Company/period/account endpoints send one property per request. Reconciliation and account IDs are resolved via the API by handle/number.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
lib/api/sfApi.js (1)
587-633: Extract the repeated per-property POST helper.
updateCompanyCustom,updatePeriodCustom, andupdateAccountCustomall duplicate the same request/collect/error-handling loop. Pulling that into one helper will keep response handling consistent when one of these endpoints inevitably changes.♻️ Possible refactor
+async function postCustomProperties(instance, url, properties) { + const results = []; + for (const prop of properties) { + try { + const response = await instance.post(url, prop); + apiUtils.responseSuccessHandler(response); + results.push(response); + } catch (error) { + const response = await apiUtils.responseErrorHandler(error); + results.push(response); + } + } + return results; +} + async function updateCompanyCustom(firmId, companyId, properties) { const instance = AxiosFactory.createInstance("firm", firmId); - const results = []; - for (const prop of properties) { - try { - const response = await instance.post(`/companies/${companyId}/custom`, prop); - apiUtils.responseSuccessHandler(response); - results.push(response); - } catch (error) { - const response = await apiUtils.responseErrorHandler(error); - results.push(response); - } - } - return results; + return postCustomProperties(instance, `/companies/${companyId}/custom`, properties); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/api/sfApi.js` around lines 587 - 633, The three functions updateCompanyCustom, updatePeriodCustom, and updateAccountCustom duplicate the same per-property POST loop and error/success handling; extract that loop into a shared helper (e.g., postPropertiesWithCollect or postPerPropertyCustom) that accepts an Axios instance, a URL (or URL builder), and the properties array, iterates over properties, calls instance.post(url, prop), invokes apiUtils.responseSuccessHandler on success and apiUtils.responseErrorHandler on catch, collects each response into an array and returns it; then replace the bodies of updateCompanyCustom, updatePeriodCustom, and updateAccountCustom to call this helper with the appropriate URL for company, period, and account.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli.js`:
- Around line 541-543: The code parses the target URL into urlData via
liquidTestUtils.extractURL (producing firmId, companyId and the target
period/reconciliation/account identity), but the update-building logic (the
loops that assemble the updates array and the fiscal_year.end_date matching
logic) currently selects records by fiscal_year.end_date and enqueues every
block found; change that logic to filter using the identity fields extracted
from options.url/urlData (e.g., periodId, reconciliationId, accountId or
equivalent) so only records whose period/reconciliation/account IDs match
urlData are included when constructing updates, and ensure subsequent
update/enqueue code uses firmId/companyId from urlData for scoping rather than
broadly matching fiscal_year.end_date.
- Around line 603-609: The current guard checks the truthiness of the value
returned by SF.findAccountByNumber, but that function can return an error object
(from apiUtils.responseErrorHandler) which is truthy and will cause accessing
account.account.id to throw; update the conditional after calling
SF.findAccountByNumber to explicitly verify the resolved shape and existence of
account.account.id (e.g., ensure account && account.account &&
account.account.id) before calling SF.updateAccountCustom, and if missing log a
descriptive error (using consola.error) and return null so lookup failures and
error objects are handled cleanly.
- Around line 631-643: The loop that applies uploads in bin/cli.js (iterating
over updates and calling update.apply()) currently logs failures but never sets
a non-zero exit status; modify the end of the loop (or after the loop completes)
to set process.exitCode = 1 when any response in responses is non-2xx (i.e.,
when failed.length > 0) so CI sees failure; alternatively throw an Error after
detecting failures — update the logic around the variables update, responses,
and failed to ensure the process exits non-zero on any upload failure.
In `@lib/utils/textPropertyUtils.js`:
- Around line 60-75: The current loop (using handleDirs, testsDir, yamlFiles and
checking parsed[testName]) returns the first YAML that contains
parsed[testName], which causes nondeterministic selection when handle is
omitted; instead, collect all matching filePath/parsed pairs across handleDirs
and only return when there is exactly one unique match, otherwise disambiguate
by preferring matches in the directory that equals the provided handle (variable
handle) or throw a clear error asking the caller to specify a handle; apply the
same change to the similar logic around lines 120-125 so both code paths gather
all matches, enforce uniqueness, and only resolve when a single unambiguous
match remains.
- Around line 31-36: The code currently collapses deep dotted keys into a single
string key by using subKey = keyParts.slice(2).join("."), causing e.g.
ns.key.a.b to become { "a.b": value } instead of a nested object; update the
logic where namespaceMap is populated (look for namespaceMap, namespaceKey,
keyParts, subKey) to build real nested objects for keyParts.slice(2) by
iterating the remaining segments and creating nested plain objects until the
final segment where you assign value (instead of joining with "."); ensure
namespaceMap.get(namespaceKey).value is mutated to contain the proper nested
structure rather than a single dotted-key entry.
---
Nitpick comments:
In `@lib/api/sfApi.js`:
- Around line 587-633: The three functions updateCompanyCustom,
updatePeriodCustom, and updateAccountCustom duplicate the same per-property POST
loop and error/success handling; extract that loop into a shared helper (e.g.,
postPropertiesWithCollect or postPerPropertyCustom) that accepts an Axios
instance, a URL (or URL builder), and the properties array, iterates over
properties, calls instance.post(url, prop), invokes
apiUtils.responseSuccessHandler on success and apiUtils.responseErrorHandler on
catch, collects each response into an array and returns it; then replace the
bodies of updateCompanyCustom, updatePeriodCustom, and updateAccountCustom to
call this helper with the appropriate URL for company, period, and account.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa47adc3-f595-435c-8c10-00b968bb4ef5
📒 Files selected for processing (3)
bin/cli.jslib/api/sfApi.jslib/utils/textPropertyUtils.js
Benjvandam
force-pushed
the
implement-reverse-test
branch
from
32eb811 to
06d247e
Compare
# Conflicts: # CHANGELOG.md # package-lock.json # package.json
michieldegezelle
left a comment
michieldegezelle
left a comment
Contributor
There was a problem hiding this comment.
🟠 Major — Missing test file: lib/utils/textPropertyUtils.js
Every other file in tests/lib/utils/ has a corresponding *.test.js (liquidTestUtils.test.js, urlHandler.test.js, etc.), but textPropertyUtils.js has none. The two functions have non-trivial logic worth covering:
transformCustomToProperties: key splitting, silent-drop behaviour for duplicate/conflicting dotted keys, mixed-depth keysfindTestData: multi-directory scan, first-match selection, edge cases on missing test name
Without tests, regressions in parsing logic will only surface when users run the real command against a live company.
Resolve conflicts: take main's package.json/package-lock (drop the branch's version bump per the 'skip bumping version' checklist), union .gitignore, and move the update-text-properties changelog note under [Unreleased].
- R1: paginate company periods via getAllPeriods so companies with >200 periods no longer silently skip updates - R2: guard null fiscal_year when matching periods to YAML period keys - R3: skip malformed YAML test files (try/catch) instead of crashing - R4: add unit tests for textPropertyUtils (transform + findTestData) - R5: clarify the command description to reflect all-level writes and add a confirmation prompt summarising the target firm/levels (--yes to skip) - R6: validate firm/company parsed from the URL
Benjvandam
added a commit
that referenced
this pull request
Jun 29, 2026
Consolidates the live-bridge read commands onto this branch so the four new commands (get-results, capture, set-custom, delete-custom) ship together on top of update-text-properties (#249). get-results reads a live file's computed results+customs; capture snapshots a live file as JSON (scoped, or --full). Adds lib/resultsReader.js, lib/dataCapture.js (+ buildLiquidTest refactor of liquidTestGenerator), and their tests.
- updatePeriodCustom / updateAccountCustom now POST a single bulk
{ properties: [...] } body, matching updateReconciliationCustom. Verified
live: the period/reconciliation/account /custom endpoints accept the bulk
shape (201); the company /custom endpoint does NOT (400 'namespace is
missing...'), so updateCompanyCustom keeps its per-property loop, with a
comment documenting the API difference.
- New apiUtils.batchResponseErrorHandler: logs and returns the error response
instead of process.exit(1) on 422/403, so one failed write inside
update-text-properties is counted via hadFailures and the remaining updates
still run (was: the whole CLI aborted mid-batch).
- update-text-properties exit codes: an unparseable URL and a period listed in
the YAML but absent from the company (incl. getAllPeriods returning []) now
set process.exitCode = 1, consistent with the reconciliation/account
not-found paths — a partially seeded scenario is detectable by scripts/CI.
| let hadFailures = false; | ||
| for (const update of updates) { | ||
| consola.start(`Updating ${update.level} (${update.properties.length} properties)...`); | ||
| const response = await update.apply(); |
Contributor
There was a problem hiding this comment.
🟠 Major: await update.apply() isn't wrapped in a try/catch, so a thrown error from the reconciliation/account lookup helpers aborts the entire remaining batch — the same failure mode this PR just fixed for the write calls via batchResponseErrorHandler.
Concretely, apply() for reconciliation/account updates (lines 597-598, 614-615) calls SF.findReconciliationInWorkflows / SF.findAccountByNumber, which still resolve through the old apiUtils.responseErrorHandler:
- On 403/422 it calls
process.exit(1)directly — killing the whole command mid-batch. - On any other unhandled status (401, 5xx) it rethrows, which isn't caught anywhere in this loop, so it becomes an uncaught exception (
errorUtils.uncaughtErrors→process.exit(1)). - If
getWorkflows(called fromfindReconciliationInWorkflows) returns an error response instead of throwing (e.g. 404),responseWorkflows.dataisundefined, andfor (const workflow of responseWorkflows.data)throws a TypeError.
Any of these mid-batch turns what should be one failed update (tracked via hadFailures) into a hard crash that silently drops every remaining update in the run — undermining the batch-continuation goal this PR just implemented for the direct write path.
Suggested fix — wrap the resolve+apply step so lookup failures are treated the same as write failures:
for (const update of updates) {
consola.start(`Updating ${update.level} (${update.properties.length} properties)...`);
let response;
try {
response = await update.apply();
} catch (error) {
hadFailures = true;
consola.error(`${update.level}: failed to resolve/update — ${error.message}`);
continue;
}
if (!response) {
hadFailures = true;
continue;
}
...
}
Contributor
Author
There was a problem hiding this comment.
Fixed — the apply loop now wraps update.apply() in a try/catch: a thrown error (network failure, lookup 500) is logged, counted as a failure (exit code 1) and the batch continues with the remaining updates. Also guarded findReconciliationInWorkflows against an undefined workflows response, so a handled 404 warns "not found" instead of throwing.
Contributor
|
Tested it on this link: https://live.getsilverfin.com/f/1355/1452520/ledgers/32064351/workflows/3125477/reconciliation_texts/87911771?current_ledger_id=32064351 And got the following: 
It seemed to select 2023 only, even though my provided URL was on 2025. Any clue why this is happening? 2023 was overwritten correctly though |
…roperties - Resolve YAML period keys via findPeriodByKey: match period ids (the capture fallback key for periods without a fiscal year), prefer the year-end period when several periods share a fiscal_year.end_date, and fail the entry instead of writing to an arbitrary period when the key stays ambiguous. - Wrap the apply loop in try/catch so a thrown error (network failure, lookup 500) counts as a per-item failure with exit code 1 instead of killing the process mid-batch without a summary. - Guard findReconciliationInWorkflows against an undefined workflows response (handled 404) so it warns 'not found' instead of throwing.
…override The same test name can exist in several YAML files in one tests/ folder (base + TY24/TY25 year variants) with different periods and data, and findTestData silently used the first file in directory order — seeding data from the wrong variant. findTestData now reads the file referenced by the template config.json "test" key (the same file run-test uses). A new --file <exact-file-name> option overrides it to select a specific variant. When the test name matches in several templates, the command lists the candidates and asks for --handle instead of guessing.
Contributor
Author
|
The URL only identifies the firm and company — the periods come from the YAML test itself. The test name exists in three files in that tests/ folder (base, TY24, TY25) and the command took the first match in directory order: the TY24 file, whose period key is 2023-12-31. That's why 2023 was written (correctly, from the TY24 data) regardless of the ledger in the URL. Reworked in the latest commits: the command now reads the test file referenced by the template's config.json |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes # #242
Description
New
update-text-propertiescommand that uploads custom text properties from a Liquid Test YAML file to a reconciliation in a company file. It extracts custom data from all 4 levels: company, period, reconciliation, and account.Supports
--handlefor faster YAML file lookup and--dry-runto preview the payload without uploading.Testing Instructions
Steps:
be_market) that has reconciliation templates with liquid test YAML filesdata.company.custom) and verify those are also pushed--handleto verify it scans all template folders and finds the test automaticallyAuthor Checklist
Reviewer Checklist