-
Notifications
You must be signed in to change notification settings - Fork 2
Implement reverse test #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4c3a0e5
0c5811d
d48e328
6fe5858
b3b059e
40984d8
fd48970
bfc7a5c
06d247e
ec00fec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| node_modules | ||
| .env | ||
| .DS_Store | ||
| ./tmp | ||
| ./tmp | ||
| .cursor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ const { runCommandChecks } = require("../lib/cli/utils"); | |
| const { CwdValidator } = require("../lib/cli/cwdValidator"); | ||
| const { AutoCompletions } = require("../lib/cli/autoCompletions"); | ||
| const fsUtils = require("../lib/utils/fsUtils"); | ||
| const textPropertyUtils = require("../lib/utils/textPropertyUtils"); | ||
| const liquidTestUtils = require("../lib/utils/liquidTestUtils"); | ||
|
|
||
| const firmIdDefault = cliUtils.loadDefaultFirmId(); | ||
| cliUtils.handleUncaughtErrors(); | ||
|
|
@@ -527,6 +529,129 @@ program | |
| liquidTestGenerator.testGenerator(options.url, testName, reconciledStatus); | ||
| }); | ||
|
|
||
| // Update Text Properties from Liquid Test data | ||
| program | ||
| .command("update-text-properties") | ||
| .description("Upload custom text properties from a Liquid Test YAML file to a reconciliation in a company file") | ||
| .requiredOption("-u, --url <url>", "Specify the full Silverfin URL of the reconciliation in the company file (mandatory)") | ||
| .requiredOption("-t, --test <test-name>", "Specify the name of the test to use as data source (mandatory)") | ||
| .option("-h, --handle <handle>", "Specify the reconciliation handle to narrow down the YAML file search (optional)") | ||
| .option("--dry-run", "Only transform and display the properties without uploading (optional)", false) | ||
| .action(async (options) => { | ||
| // Parse URL to extract IDs | ||
| const urlData = liquidTestUtils.extractURL(options.url); | ||
| const { firmId, companyId } = urlData; | ||
|
Comment on lines
+541
to
+543
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scope period/reconciliation/account updates to the URL target. Lines 542-543 parse a specific reconciliation URL, but Line 567 switches to matching by Also applies to: 561-613 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of the command is to set up a full test scenario in a company file — that requires pushing custom data to all levels and templates referenced in the test, not just the single reconciliation from the URL. The URL is used to identify the target company/firm/period, but the test YAML defines the complete data set needed for that scenario to work. |
||
|
|
||
| // Find the test data in the YAML files | ||
| const testData = textPropertyUtils.findTestData(options.test, options.handle); | ||
| consola.info(`Found test "${options.test}" in ${testData.handle}/tests/${testData.file}`); | ||
|
|
||
| // Collect all updates to perform | ||
| const updates = []; | ||
|
|
||
| // Company-level custom | ||
| if (testData.company?.custom) { | ||
| const properties = textPropertyUtils.transformCustomToProperties(testData.company.custom); | ||
| updates.push({ level: "company", properties, apply: () => SF.updateCompanyCustom(firmId, companyId, properties) }); | ||
| } | ||
|
|
||
| // Fetch periods once if needed for resolving period dates | ||
| let periodsArray = null; | ||
|
|
||
| for (const [periodKey, periodEntry] of Object.entries(testData.periods)) { | ||
| // Resolve period date to period ID | ||
| if (!periodsArray) { | ||
| const periodsResponse = await SF.getPeriods(firmId, companyId); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Major: let periodsArray = [];
let page = 1;
while (true) {
const resp = await SF.getPeriods(firmId, companyId, page);
const batch = resp?.data || [];
periodsArray = periodsArray.concat(batch);
if (batch.length < 200) break;
page++;
} |
||
| periodsArray = periodsResponse?.data || []; | ||
| } | ||
| const period = periodsArray.find((p) => p.fiscal_year.end_date === periodKey); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor: const period = periodsArray.find((p) => p.fiscal_year?.end_date === periodKey); |
||
| if (!period) { | ||
| consola.warn(`Period "${periodKey}" not found in company — skipping`); | ||
| continue; | ||
| } | ||
| const targetPeriodId = period.id; | ||
|
|
||
| // Period-level custom | ||
| if (periodEntry.custom) { | ||
| const properties = textPropertyUtils.transformCustomToProperties(periodEntry.custom); | ||
| updates.push({ level: `period [${periodKey}]`, properties, apply: () => SF.updatePeriodCustom(firmId, companyId, targetPeriodId, properties) }); | ||
| } | ||
|
|
||
| // Reconciliation-level custom | ||
| for (const [reconHandle, customData] of Object.entries(periodEntry.reconciliations)) { | ||
| const properties = textPropertyUtils.transformCustomToProperties(customData); | ||
| updates.push({ | ||
| level: `reconciliation [${reconHandle}] in ${periodKey}`, | ||
| properties, | ||
| apply: async () => { | ||
| const recon = await SF.findReconciliationInWorkflows(firmId, reconHandle, companyId, targetPeriodId); | ||
| if (!recon) { | ||
| consola.error(`Reconciliation "${reconHandle}" not found in any workflow for period ${periodKey}`); | ||
| return null; | ||
| } | ||
| return SF.updateReconciliationCustom(firmId, companyId, targetPeriodId, recon.id, properties); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| // Account-level custom | ||
| for (const [accountNumber, customData] of Object.entries(periodEntry.accounts)) { | ||
| const properties = textPropertyUtils.transformCustomToProperties(customData); | ||
| updates.push({ | ||
| level: `account [${accountNumber}] in ${periodKey}`, | ||
| properties, | ||
| apply: async () => { | ||
| const account = await SF.findAccountByNumber(firmId, companyId, targetPeriodId, accountNumber); | ||
| const accountId = account?.account?.id; | ||
| if (!accountId) { | ||
| consola.error(`Account "${accountNumber}" could not be resolved for period ${periodKey}`); | ||
| return null; | ||
| } | ||
| return SF.updateAccountCustom(firmId, companyId, targetPeriodId, accountId, properties); | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (updates.length === 0) { | ||
| consola.warn("No custom properties found in this test"); | ||
| return; | ||
| } | ||
|
|
||
| consola.info(`Found ${updates.length} custom update(s) to apply`); | ||
|
|
||
| if (options.dryRun) { | ||
| for (const update of updates) { | ||
| consola.info(`[dry-run] ${update.level}: ${update.properties.length} properties`); | ||
| consola.log(JSON.stringify(update.properties, null, 2)); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Apply all updates | ||
| let hadFailures = false; | ||
| for (const update of updates) { | ||
| consola.start(`Updating ${update.level} (${update.properties.length} properties)...`); | ||
| const response = await update.apply(); | ||
| if (!response) { | ||
| hadFailures = true; | ||
| continue; | ||
| } | ||
| // Handle both single response (reconciliation) and array of responses (company/period/account) | ||
| const responses = Array.isArray(response) ? response : [response]; | ||
| const failed = responses.filter((r) => !r || r.status < 200 || r.status >= 300); | ||
| if (failed.length === 0) { | ||
| consola.success(`${update.level}: updated`); | ||
| } else { | ||
| hadFailures = true; | ||
| consola.error(`${update.level}: ${failed.length}/${responses.length} failed`); | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| if (hadFailures) { | ||
| process.exitCode = 1; | ||
| } | ||
| }); | ||
|
|
||
| // Check Liquid Test dependencies for a reconciliation template | ||
| program | ||
| .command("check-dependencies") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const yaml = require("yaml"); | ||
| const { consola } = require("consola"); | ||
| const fsUtils = require("./fsUtils"); | ||
|
|
||
| /** | ||
| * Transform a YAML custom properties object into the Silverfin API format. | ||
| * Input: flat object with dot-notation keys (e.g. "namespace.key.subkey": value) | ||
| * Output: array of { namespace, key, value } objects | ||
| */ | ||
| function transformCustomToProperties(customData) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion: This module contains non-trivial transformation and YAML lookup logic ( |
||
| const namespaceMap = new Map(); | ||
|
|
||
| for (const [fullKey, value] of Object.entries(customData)) { | ||
| const keyParts = fullKey.split("."); | ||
|
|
||
| if (keyParts.length < 2) { | ||
| consola.warn(`Skipping key "${fullKey}" — expected namespace.key format`); | ||
| continue; | ||
| } | ||
|
|
||
| const namespace = keyParts[0]; | ||
| const key = keyParts[1]; | ||
| const namespaceKey = `${namespace}.${key}`; | ||
|
|
||
| if (keyParts.length === 2) { | ||
| if (!namespaceMap.has(namespaceKey)) { | ||
| namespaceMap.set(namespaceKey, { namespace, key, value }); | ||
| } | ||
| } else { | ||
| if (!namespaceMap.has(namespaceKey)) { | ||
| namespaceMap.set(namespaceKey, { namespace, key, value: {} }); | ||
| } | ||
| const subKey = keyParts.slice(2).join("."); | ||
| namespaceMap.get(namespaceKey).value[subKey] = value; | ||
|
Comment on lines
+31
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Build real nested objects for deep dotted keys. Lines 35-36 collapse the remaining path into a single 🐛 Possible fix } else {
if (!namespaceMap.has(namespaceKey)) {
namespaceMap.set(namespaceKey, { namespace, key, value: {} });
}
- const subKey = keyParts.slice(2).join(".");
- namespaceMap.get(namespaceKey).value[subKey] = value;
+ let current = namespaceMap.get(namespaceKey).value;
+ for (const part of keyParts.slice(2, -1)) {
+ if (typeof current[part] !== "object" || current[part] === null) {
+ current[part] = {};
+ }
+ current = current[part];
+ }
+ current[keyParts[keyParts.length - 1]] = value;
}🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 4 nesting levels doesn't happen in practice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
|
||
| } | ||
|
Comment on lines
+27
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential data inconsistency when mixing simple and nested keys for the same namespace.key. If the input contains both a simple key (e.g.,
This could lead to unexpected behavior depending on object property iteration order. 🛡️ Proposed fix to handle mixed keys consistently if (keyParts.length === 2) {
- if (!namespaceMap.has(namespaceKey)) {
- namespaceMap.set(namespaceKey, { namespace, key, value });
- }
+ // Simple key always sets the value directly (overwrite if exists)
+ const existing = namespaceMap.get(namespaceKey);
+ if (existing && typeof existing.value === "object") {
+ consola.warn(`Key "${fullKey}" conflicts with nested keys under same namespace.key — skipping`);
+ continue;
+ }
+ namespaceMap.set(namespaceKey, { namespace, key, value });
} else {🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't happen in the YAML files in practice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
🧠 Learnings used |
||
| } | ||
|
|
||
| return Array.from(namespaceMap.values()); | ||
| } | ||
|
|
||
| /** | ||
| * Find a test by name across YAML files in a template's tests/ folder. | ||
| * If handle is provided, only search that handle's folder. | ||
| * If not, scan all reconciliation_texts folders. | ||
| * Extracts custom data from all 4 levels: company, period, reconciliation, account. | ||
| * Returns { file, handle, company, periods } where periods contains per-period custom, | ||
| * reconciliation custom, and account custom data. | ||
| */ | ||
| function findTestData(testName, handle) { | ||
| const templateType = "reconciliationText"; | ||
| const baseDir = path.join(process.cwd(), fsUtils.FOLDERS[templateType]); | ||
|
|
||
| if (!fs.existsSync(baseDir)) { | ||
| consola.error(`Directory not found: ${fsUtils.FOLDERS[templateType]}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const handleDirs = handle ? [handle] : fs.readdirSync(baseDir).filter((entry) => { | ||
| return fs.statSync(path.join(baseDir, entry)).isDirectory(); | ||
| }); | ||
|
|
||
| for (const dir of handleDirs) { | ||
| const testsDir = path.join(baseDir, dir, "tests"); | ||
| if (!fs.existsSync(testsDir)) continue; | ||
|
|
||
| const yamlFiles = fs.readdirSync(testsDir).filter((f) => f.endsWith(".yml")); | ||
|
|
||
| for (const file of yamlFiles) { | ||
| const filePath = path.join(testsDir, file); | ||
| const content = fs.readFileSync(filePath, "utf-8"); | ||
| const parsed = yaml.parse(content, { maxAliasCount: 10000, merge: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor: let parsed;
try {
parsed = yaml.parse(content, { maxAliasCount: 10000, merge: true });
} catch (e) {
consola.warn(`Skipping ${filePath}: invalid YAML — ${e.message}`);
continue;
}
if (!parsed || !parsed[testName]) continue; |
||
|
|
||
| if (!parsed || !parsed[testName]) continue; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| const testData = parsed[testName]; | ||
| const result = { file, handle: dir, company: null, periods: {} }; | ||
|
|
||
| // Company-level custom | ||
| if (testData?.data?.company?.custom) { | ||
| result.company = { custom: testData.data.company.custom }; | ||
| } | ||
|
|
||
| // Period-level data | ||
| const periods = testData?.data?.periods; | ||
| if (periods) { | ||
| for (const [periodKey, periodData] of Object.entries(periods)) { | ||
| if (!periodData) continue; | ||
|
|
||
| const periodEntry = { custom: null, reconciliations: {}, accounts: {} }; | ||
|
|
||
| // Period-level custom | ||
| if (periodData.custom) { | ||
| periodEntry.custom = periodData.custom; | ||
| } | ||
|
|
||
| // Reconciliation-level custom | ||
| if (periodData.reconciliations) { | ||
| for (const [reconHandle, reconData] of Object.entries(periodData.reconciliations)) { | ||
| if (reconData?.custom) { | ||
| periodEntry.reconciliations[reconHandle] = reconData.custom; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Account-level custom | ||
| if (periodData.accounts) { | ||
| for (const [accountNumber, accountData] of Object.entries(periodData.accounts)) { | ||
| if (accountData?.custom) { | ||
| periodEntry.accounts[accountNumber] = accountData.custom; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.periods[periodKey] = periodEntry; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } | ||
|
|
||
| consola.error(`Test "${testName}" not found in any YAML file`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| module.exports = { transformCustomToProperties, findTestData }; | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Major: The command description says properties are uploaded "to a reconciliation in a company file", but the implementation pushes custom data at company, period, reconciliation, and account levels for every entry in the test YAML. That mismatch will surprise users who paste a reconciliation URL expecting a single-target update.
Either narrow the scope to the URL's period/reconciliation (as CodeRabbit suggested) or update the description/CHANGELOG to state explicitly that this seeds the full test scenario across all YAML levels.