Create tests for account templates #234
Conversation
WalkthroughThis pull request introduces support for account templates alongside existing reconciliation text templates. Three new API functions fetch account template data and locate accounts by number. Core generator and utility logic now branch on template type, with comprehensive test coverage for both template types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (4)
lib/api/sfApi.js (1)
407-417: Use thePER_PAGEconstant instead of hardcoded value.Line 410 uses a hardcoded
200while the rest of the file consistently uses thePER_PAGEconstant defined at line 7. This inconsistency could lead to maintenance issues.- const response = await instance.get(`/companies/${companyId}/periods/${periodId}/accounts/${accountTemplateId}/custom`, { params: { page: page, per_page: 200 } }); + const response = await instance.get(`/companies/${companyId}/periods/${periodId}/accounts/${accountTemplateId}/custom`, { params: { page: page, per_page: PER_PAGE } });lib/utils/liquidTestUtils.js (3)
16-16: Clarify or relocate the comment.The comment "Only for reconciliation texts" is misleading because the
reconciliationsobject is present in the base structure but conditionally removed for account templates on line 31. Consider moving this comment next to the deletion logic or rewording it for clarity.Apply this diff to improve clarity:
periods: { replace_period_name: { - reconciliations: {}, // Only for reconciliation texts + reconciliations: {}, }, }, },And add a clearer comment near the deletion:
// Add current_account for account templates if (templateType === "accountTemplate") { baseStructure[testName].context.current_account = "#Replace with current account"; - delete baseStructure[testName].data.periods.replace_period_name.reconciliations; // Remove reconciliations + delete baseStructure[testName].data.periods.replace_period_name.reconciliations; // Only for reconciliation texts }
163-163: Remove or complete the comment.The comment "// Fori" appears incomplete or erroneous. Either complete it or remove it.
Apply this diff:
const obj = {}; for (const item of sortedArray) { const element = `${item.namespace}.${item.key}`; - // Fori if (item.value && item.value.field) { obj[element] = item.value.field; } else {
175-207: Consider consistent parameter naming across functions.The
getCompanyDependenciesfunction was updated to use generic parameter names (templateCode,templateHandle), but other similar functions (searchForResultsFromDependenciesInLiquid,searchForCustomsFromDependenciesInLiquid,lookForSharedPartsInLiquid) still usereconcilationObjectandreconciliationHandle. Additionally, there's a typo: "reconcilationObject" should be "reconciliationObject" (missing 'i').For better maintainability and consistency, consider:
- Fixing the typo in all occurrences
- Using consistent parameter names across similar functions (either keep reconciliation-specific names or use generic template names)
Example for fixing the typo in
searchForResultsFromDependenciesInLiquid:-function searchForResultsFromDependenciesInLiquid(reconcilationObject, reconciliationHandle, resultsCollection = {}) { +function searchForResultsFromDependenciesInLiquid(reconciliationObject, reconciliationHandle, resultsCollection = {}) { // Normal Scenario const reResultsFetched = RegExp(/period\.reconciliations\.\w+\.results\.\w+/g); // period.reconciliations.handle.results.result_name // No main part ? - if (!reconcilationObject || !reconcilationObject.text) { + if (!reconciliationObject || !reconciliationObject.text) { consola.warn(`Reconciliation "${reconciliationHandle}": no liquid code found`);Apply similar changes to
searchForCustomsFromDependenciesInLiquidandlookForSharedPartsInLiquid.Also applies to: 211-379
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/api/sfApi.js(4 hunks)lib/liquidTestGenerator.js(4 hunks)lib/utils/liquidTestUtils.js(2 hunks)tests/lib/liquidTestGenerator.test.js(8 hunks)tests/lib/templates/accountTemplates.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/liquidTestUtils.js (4)
lib/liquidTestGenerator.js (2)
templateType(13-13)liquidTestObject(16-16)lib/utils/fsUtils.js (3)
handle(238-238)handle(266-266)fs(1-1)lib/cli/devMode.js (3)
filePath(25-25)fs(1-1)fsUtils(5-5)lib/liquidTestRunner.js (4)
filePath(295-295)filePath(302-302)fs(8-8)fsUtils(13-13)
tests/lib/liquidTestGenerator.test.js (2)
lib/api/sfApi.js (3)
require(2-2)require(3-3)require(4-4)lib/liquidTestGenerator.js (7)
require(2-2)require(4-4)require(5-5)require(6-6)require(7-7)Utils(3-3)SF(1-1)
🔇 Additional comments (10)
lib/api/sfApi.js (1)
419-429: LGTM!The function follows the established pattern for API calls in this file.
lib/liquidTestGenerator.js (3)
27-57: LGTM!The template type branching is well-structured with appropriate error handling for account templates.
100-133: LGTM!The template-type-specific handling for customs and results is correctly implemented, using appropriate API calls for each template type.
151-247: LGTM!Dependency resolution is correctly scoped to
reconciliationTextonly, aligning with the PR description that states dependency checks are not implemented for account templates and may be added later.tests/lib/templates/accountTemplates.test.js (1)
16-17: LGTM!Test fixtures correctly updated to include
name_enandname_frfields, maintaining consistency with the account template data structure.tests/lib/liquidTestGenerator.test.js (2)
346-618: Excellent test coverage for account template test generation.The new test suite comprehensively covers:
- Account lookup and template detail retrieval
- Context population with
current_account- Custom and results data fetching
- Starred status from account response
- Dependency resolution being skipped for account templates
- Error handling for missing associations, missing templates, and lookup failures
- Period custom data processing
173-175: LGTM!Updated assertions correctly reflect the new
exportYAMLsignature that includestemplateType.lib/utils/liquidTestUtils.js (3)
39-65: LGTM!The URL extraction logic correctly identifies template types and returns the appropriate ID field names. Error handling is well-implemented.
67-88: LGTM!The file name generation with templateType-based path branching and recursive uniqueness checking is well-implemented. Good error handling for invalid template types.
91-125: LGTM!The YAML export function properly handles templateType with appropriate folder creation and validation. The implementation is clean and well-structured.
|
|
||
| ## [1.38.0] (04/07/2025) | ||
| - Added a changelog.md file and logic to display the changes when updating to latest version | ||
| - Added a changelog.md file and logic to display the changes when updating to latest version |
There was a problem hiding this comment.
Missing changelog entry for the new account template test creation feature.
This PR introduces significant new functionality (create-test support for account templates), but the CHANGELOG does not document this change. Consider adding an entry similar to:
+## [1.49.0] (17/12/2025)
+- Added `create-test` command support for account templates (fetches template data, period data, and custom data)
+
## [1.48.0] (25/09/2025)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In CHANGELOG.md around line 54, the changelog is missing an entry for the new
"create-test" support for account templates; add a new changelog entry under the
appropriate unreleased or version section with the release/version header and
date (if applicable) and a concise bullet summarizing the feature (e.g., "Added
create-test support for account templates — enables creating test instances from
account templates via X API/CLI"), include PR number and author/maintainer
initials if available, and place it in the correct section (Added/Changed) so
the change is recorded for release notes.
| async function findAccountByNumber(firmId, companyId, periodId, accountNumber, page = 1) { | ||
| const instance = AxiosFactory.createInstance("firm", firmId); | ||
| try { | ||
| const response = await instance.get(`companies/${companyId}/periods/${periodId}/accounts`, { | ||
| params: { page: page }, | ||
| }); | ||
|
|
||
| const accounts = response.data; | ||
|
|
||
| // No data - end of pagination | ||
| if (accounts.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| // Look for the account in this page | ||
| const account = accounts.find((acc) => acc.account.number === accountNumber); | ||
| if (account) { | ||
| return account; | ||
| } | ||
|
|
||
| // Not found in this page, try next page | ||
| return findAccountByNumber(firmId, companyId, periodId, accountNumber, page + 1); | ||
| } catch (error) { | ||
| apiUtils.responseErrorHandler(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing return statement in error handler will silently swallow errors.
The catch block calls apiUtils.responseErrorHandler(error) but doesn't return or rethrow, causing the function to implicitly return undefined. This masks errors and makes debugging difficult.
} catch (error) {
- apiUtils.responseErrorHandler(error);
+ const response = await apiUtils.responseErrorHandler(error);
+ return response;
}
}Additionally, consider adding a MAX_PAGES limit similar to getAllPeriodCustom to prevent unbounded recursion if the API returns unexpected data.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/api/sfApi.js around lines 642 to 667, the catch block calls
apiUtils.responseErrorHandler(error) but neither returns its result nor
rethrows, causing the function to implicitly return undefined and swallow
errors; update the catch to rethrow or return the handled error (e.g., return
apiUtils.responseErrorHandler(error) or throw the error after handling) so
callers get an explicit failure, and add a MAX_PAGES limit (mirror
getAllPeriodCustom) by introducing a constant and checking the page parameter to
stop recursion and return null or throw a clear error when the limit is exceeded
to avoid unbounded recursion.
| let starredStatus; | ||
| switch (templateType) { | ||
| case "reconciliationText": | ||
| starredStatus = { | ||
| ...SF.findReconciliationInWorkflow(parameters.firmId, templateHandle, parameters.companyId, parameters.ledgerId, parameters.workflowId), | ||
| }.starred; | ||
| break; | ||
| case "accountTemplate": | ||
| starredStatus = responseDetails.starred; // Already present in the response of the Account | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing await causes starredStatus to always be undefined for reconciliations.
SF.findReconciliationInWorkflow is an async function, but the call is not awaited. Spreading the Promise object and accessing .starred will always return undefined.
let starredStatus;
switch (templateType) {
case "reconciliationText":
starredStatus = {
- ...SF.findReconciliationInWorkflow(parameters.firmId, templateHandle, parameters.companyId, parameters.ledgerId, parameters.workflowId),
+ ...(await SF.findReconciliationInWorkflow(parameters.firmId, templateHandle, parameters.companyId, parameters.ledgerId, parameters.workflowId)),
}.starred;
break;🤖 Prompt for AI Agents
In lib/liquidTestGenerator.js around lines 60 to 70, the switch branch for
"reconciliationText" spreads the result of SF.findReconciliationInWorkflow
without awaiting it, so starredStatus becomes undefined; change the call to
await the async function (e.g., const rec = await
SF.findReconciliationInWorkflow(...); starredStatus = { ...rec }.starred) and
ensure the enclosing function is declared async or otherwise handles the awaited
call; keep the "accountTemplate" branch as-is.
| const periodCustoms = Utils.processCustom(allPeriodCustoms); | ||
| liquidTestObject[testName].data.periods[currentPeriodData.fiscal_year.end_date].custom = periodCustoms; | ||
| } | ||
| process.exit; |
There was a problem hiding this comment.
process.exit; is a no-op — missing parentheses for function call.
This line references process.exit without invoking it. If the intent is to exit, add parentheses. If it's leftover debug code, remove it entirely.
- process.exit;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.exit; |
🤖 Prompt for AI Agents
In lib/liquidTestGenerator.js around line 98, the statement "process.exit;" is a
no-op; either remove this leftover debug line or invoke the function correctly.
If the intent is to terminate the process on error, replace it with a call such
as process.exit(1) (or process.exit(0) for normal exit); otherwise simply delete
the line.
Fixes # (link to the corresponding issue if applicable)
Description
Make sure that the create test functionality works for account templates as well.
Limitations: functionality is limited to fetching the template data/period data/custom data. We won't check for template dependencies for now since this is rather rare.
If needed, that can be added later on.
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist