Create tests for account templates #234
Conversation
WalkthroughAdds account-template support alongside reconciliation-text: new SF API endpoints and a paginated account lookup, templateType-aware generator and utils (signature changes for base test creation and YAML export), expanded tests for account-template flows, package version bump, and a minor CHANGELOG newline adjustment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
michieldegezelle
requested review from
BenjaminLangenakenSF
and removed request for
AgustinSilverfin
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
michieldegezelle
force-pushed
the
create-tests-for-account-templates
branch
from
5270726 to
b58780d
Compare
michieldegezelle
force-pushed
the
create-tests-for-account-templates
branch
from
27d051b to
0c4daae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/liquidTestGenerator.js (1)
291-306: Accounts object may be overwritten, losing current account data for accountTemplate.For
accountTemplate, lines 118-125 populateaccountswith the current account's data. However, iflookForAccountsIDsfinds account references in customs, line 294 unconditionally resetsaccounts = {}, potentially losing the current account data.Proposed fix - merge instead of overwrite
// Search for account ids in customs const accountIds = Utils.lookForAccountsIDs(liquidTestObject); if (accountIds.length != 0) { - liquidTestObject[testName].data.periods[currentPeriodData.fiscal_year.end_date].accounts = {}; + liquidTestObject[testName].data.periods[currentPeriodData.fiscal_year.end_date].accounts = + liquidTestObject[testName].data.periods[currentPeriodData.fiscal_year.end_date].accounts || {}; for (let accountId of accountIds) {
🤖 Fix all issues with AI agents
In `@lib/liquidTestGenerator.js`:
- Around line 146-149: The exit on missing template uses process.exit() which
returns code 0; update the error path where templateCode is falsy (the block
referencing templateCode and templateHandle) to call process.exit(1) instead so
it consistently signals an error (match other error exits that use
process.exit(1)).
| if (!templateCode) { | ||
| consola.warn(`Template "${templateHandle}" wasn't found`); | ||
| process.exit(); |
There was a problem hiding this comment.
I think coderabbit is correct, and we should also use process.exit(1); here?
There was a problem hiding this comment.
Tested this functionally and seems we indeed still create an empty liquid test file in case of !templateCode:
BenjaminLangenakenSF
left a comment
BenjaminLangenakenSF
left a comment
There was a problem hiding this comment.
Added one final comment, but will already approve the PR 😉
3 participants
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