refactor app-simplefin.ts to make it easily testable #440
Conversation
tcrasset
force-pushed
the
tc/sinplefin-tests
branch
from
13e1104
to
8bc98de
Compare
| "eslint": "^8.33.0", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "jest": "^29.3.1", | ||
| "prettier": "^2.8.3", | ||
| "supertest": "^6.3.1", | ||
| "typescript": "^4.9.5" | ||
| "typescript": "^5.5.4" |
There was a problem hiding this comment.
Needed to update typescript to have certain flags in tsconfig.json work.
| const simplefinService = new SimpleFinService( | ||
| new SimplefinApi(new HttpsClient()), | ||
| ); |
There was a problem hiding this comment.
dependency injection, makes it easier to test
There was a problem hiding this comment.
I tried to mock simplefinService to be able to test app-simplefin.js as an integration test, but I couldn't get jest.spyOn to work, so I didn't. I'm relying on someone checking out this branch and manually testing that I haven't broken anything.
There was a problem hiding this comment.
I was able to test that, if the SimplefinAPI returns a valid account/transaction, the code in app-simplefin.js works. I just need someone to test with a real SimplefinAPI.
| } catch (e) { | ||
| serverDown(e, res); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const account = | ||
| !results?.accounts || results.accounts.find((a) => a.id === accountId); | ||
| const account = results.accounts.find((a) => a.id === accountId); |
There was a problem hiding this comment.
If no error is caught on line 100, results should always be set, as well as results.accounts (albeit it being an empty list) if there are any error. In that case, results.errors will have an error.
| @@ -164,20 +177,20 @@ app.post( | |||
|
|
|||
| let dateToUse = 0; | |||
|
|
|||
| if (trans.posted == 0) { | |||
| if (trans.isPending()) { | |||
There was a problem hiding this comment.
trans.posted is now a Date, so couldn't compare it to 0. Added a method on Transaction to do this.
| } | ||
|
|
||
| newTrans.bookingDate = new Date(dateToUse * 1000) | ||
| .toISOString() | ||
| .split('T')[0]; | ||
|
|
||
| newTrans.date = new Date(dateToUse * 1000).toISOString().split('T')[0]; | ||
| newTrans.payeeName = trans.payee; | ||
| // newTrans.payeeName = trans.payee; TODO: Is this used? |
There was a problem hiding this comment.
The payee field appears nowhere in the Simplefin documentation, so I think newTrans.payeeName ends up undefined.
There was a problem hiding this comment.
Hmm, seems this was changed not too long ago, so I guess the Simplefin docs are not quite up to date :/
| newTrans.booked = false; | ||
| dateToUse = trans.transacted_at; | ||
| dateToUse = trans.transacted_at.getTime() / 1000; |
There was a problem hiding this comment.
I didn't want to change the logic further down in this function, so I just reverted the transacted_at to use seconds instead of Date objects. getTime() returns milliseconds, so needed to divide by 1000.
| }); | ||
|
|
||
| return AccountSet.fromJson(result); | ||
| } |
There was a problem hiding this comment.
Separated the api call (fetchAccessKey()) from the state (context), so that the SimplefinAPI remains stateless, and the only state is in SimplefinContextData
There was a problem hiding this comment.
One would reuse context when trying to fetch accounts right after getting an access key, as the access key would live in context, and could be reused for further calls to the SimplefinAPI.
The SimplefinContextData class is really a proof of concept here, some fields may need to be removed, e.g. method and port, but that can happen at a later date.
const context = new SimplefinContextData(
'POST',
443,
{ 'Content-Length': 0 },
base64Token,
);
this.simplefinApi.setContext(context);
// Fetch access key
const accessKey = this.simplefinApi.fetchAccessKey().then(() => context.accessKey)
// Modify the context to add authentication
context.parseAccessKey(accessKey);
// Use the authentified context to make further calls
const accounts = await this.simplefinApi
.fetchAccounts(startDate, endDate)
.then((accounts) => accounts);There was a problem hiding this comment.
Right now, I could only test that our API still works and returns invalid token errors in an end-to-end way.
| "docker:artifacts": "chmod +x docker/download-artifacts.sh && ./docker/download-artifacts.sh", | ||
| "docker:build-common": "yarn docker:artifacts && docker build --build-arg GITHUB_TOKEN=$(gh auth token) -t actual-server-dev", | ||
| "docker:build-edge": "yarn docker:build-common -f docker/edge-ubuntu.Dockerfile .", | ||
| "docker:build-edge-alpine": "yarn docker:build-common -f docker/edge-alpine.Dockerfile .", | ||
| "docker:build-stable": "yarn docker:build-common -f docker/stable-ubuntu.Dockerfile .", | ||
| "docker:build-stable-alpine": "yarn docker:build-common -f docker/stable-alpine.Dockerfile .", | ||
| "docker:run": "docker run --rm -p 5006:5006 actual-server-dev", | ||
| "lint": "eslint . --max-warnings 0", |
There was a problem hiding this comment.
I built all the docker images and ran them, making sure the server ran and was accessible with curl.
| accessKey, | ||
| startDate, | ||
| endDate, | ||
| ); | ||
|
|
||
| res.send({ | ||
| status: 'ok', |
There was a problem hiding this comment.
I've gone over the code in the frontend that uses the accounts from the server, and made sure that the variables used there had the same name as the ones in Account (from models/account.ts)
|
@tcrasset that other PR was merged in. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
WalkthroughThe changes in this pull request encompass updates to multiple Dockerfiles, configuration files, and source code files across the application. Key modifications include the introduction of TypeScript execution capabilities via the In addition, new classes and methods have been introduced in the application code, particularly for handling interactions with the SimpleFIN API, encapsulating functionality within service-oriented architectures. This includes the creation of new models such as Configuration files have also been updated, introducing new properties for logging and enhancing the TypeScript compilation settings. The Jest testing framework has been utilized to improve test coverage across various components, ensuring robust validation of functionality and error handling. Overall, these changes reflect a significant shift towards a more modular and maintainable codebase with improved testing and configuration management. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (49)
src/app-simplefin/errors.ts (1)
1-7: Good implementation of a custom error class, with some suggestions for improvement.The overall structure and implementation of the
GenericSimplefinErrorclass are good. It extends the built-inErrorclass and provides a way to include additional error details. However, there are a few suggestions for improvement:
The error message 'GoCardless returned error' seems inconsistent with the class name
GenericSimplefinError. Consider updating either the class name or the error message for consistency.It's a good practice to override the
nameproperty in custom error classes. This helps in identifying the specific error type when logging or debugging.Consider using a more specific type for the
detailsproperty instead of the genericobjecttype. This can provide better type safety and autocompletion when working with the error details.Here's a suggested refactor addressing these points:
export class GenericSimplefinError extends Error { details: Record<string, unknown>; constructor(data: Record<string, unknown> = {}) { super('Simplefin returned error'); this.name = 'GenericSimplefinError'; this.details = data; } }This refactor:
- Updates the error message to be consistent with the class name.
- Overrides the
nameproperty.- Uses
Record<string, unknown>for thedetailsproperty, which is more specific thanobjectwhile still allowing flexibility.src/util/camelCase.ts (3)
5-7: Approve implementation with suggestions for edge cases.The
kebabToCamelfunction correctly converts basic kebab-case strings to camelCase. The implementation is concise and effective for most cases.Consider handling these edge cases for more robust conversion:
- Consecutive hyphens (e.g., "kebab--case")
- Uppercase letters (e.g., "kebab-Case")
- Leading or trailing hyphens
Here's an improved version that handles these cases:
function kebabToCamel(str: string): string { return str .replace(/^-+|-+$/g, '') // Remove leading/trailing hyphens .replace(/-+([a-z])/gi, (_, char) => char.toUpperCase()); // Handle consecutive hyphens and any case }This version is more forgiving and can handle a wider range of inputs.
9-16: Approve implementation with suggestions for nested structures and type improvements.The
transformKeysfunction correctly transforms keys of shallow objects from kebab-case to camelCase. The logic is sound and it effectively uses the helper functions.Consider the following improvements:
- Handle nested objects and arrays for deep transformation.
- Use more specific TypeScript types for better type safety.
Here's an improved version that addresses these points:
type Primitive = string | number | boolean | null | undefined; type DeepObject = { [key: string]: Primitive | DeepObject | DeepObject[] }; function transformKeys(obj: DeepObject): DeepObject { if (Array.isArray(obj)) { return obj.map(item => transformKeys(item as DeepObject)) as DeepObject[]; } if (typeof obj !== 'object' || obj === null) { return obj; } return Object.entries(obj).reduce((acc, [key, value]) => { const newKey = isKebabCase(key) ? kebabToCamel(key) : key; acc[newKey] = transformKeys(value as DeepObject); return acc; }, {} as DeepObject); }This version recursively transforms nested objects and arrays, and uses more specific TypeScript types for improved type safety.
1-18: Summary: Good utility implementation, aligns with refactoring objectives.This new utility file introduces functions for converting kebab-case to camelCase, which is likely part of the larger refactoring effort mentioned in the PR objectives. The implementation is generally correct and follows good practices, with room for some improvements as suggested in the previous comments.
The modular approach of this utility file contributes to better testability of the codebase, as these functions can be easily unit tested in isolation. This aligns well with the PR's goal of making the code more easily testable.
To further improve testability and maintainability:
- Consider adding unit tests for these utility functions if not already done.
- Ensure that the usage of these utilities in other parts of the codebase (like
app-simplefin.ts) is also refactored to be easily testable.- Document the purpose and usage of these utilities, especially if they're intended to be used across multiple parts of the application.
jest.config.json (1)
Line range hint
1-16: Overall assessment: Changes align with PR objectives but require careful consideration.The modifications to
jest.config.jsongenerally support the PR's goals of improving testability and cleaning up test output. However, some changes (particularly disabling coverage collection and enabling silent mode) may have unintended consequences on the development and debugging processes.Recommendations:
- Reconsider disabling coverage collection, or ensure there's an alternative process in place.
- Verify that silent mode won't hinder debugging efforts, and document how to re-enable verbose output when needed.
- The change to treat TypeScript files as ECMAScript modules is beneficial and supports direct testing of TypeScript files.
Please address the verification points raised in the individual comments to ensure these changes don't negatively impact your development workflow.
src/app-simplefin/models/account-set.ts (3)
1-1: Consider removing the file extension from the import statement.In TypeScript, it's conventional to omit the file extension when importing from other TypeScript files. This allows for easier refactoring and maintains consistency with how most TypeScript projects are set up.
Consider updating the import statement as follows:
-import Account from './account.ts'; +import Account from './account';
3-6: LGTM! Consider enhancing the comment for better documentation.The class declaration and properties are well-defined and align with the SimpleFin protocol. The comment provides useful context.
To improve the documentation, consider expanding the comment to briefly describe the purpose of the
AccountSetclass:- // https://www.simplefin.org/protocol.html#account-set + /** + * Represents an Account Set as defined in the SimpleFin protocol. + * Contains a collection of accounts and associated error messages. + * @see https://www.simplefin.org/protocol.html#account-set + */
13-19: LGTM! Consider adding error handling for robust JSON parsing.The
fromJsonstatic method provides a convenient way to create anAccountSetfrom JSON data. The implementation is correct, but it could benefit from additional error handling.Consider adding try-catch blocks to handle potential JSON parsing errors and invalid data structures:
static fromJson(json: string): AccountSet { try { const data = JSON.parse(json); if (!Array.isArray(data.accounts) || !Array.isArray(data.errors)) { throw new Error('Invalid AccountSet data structure'); } data.accounts = data.accounts.map((account: object) => { if (typeof account !== 'object' || account === null) { throw new Error('Invalid account data'); } return Account.fromJson(JSON.stringify(account)); }); return new AccountSet(data); } catch (error) { console.error('Error parsing AccountSet JSON:', error); return new AccountSet({ errors: [(error as Error).message], accounts: [] }); } }This implementation will handle JSON parsing errors and invalid data structures, returning an
AccountSetwith an error message if something goes wrong.docker/stable-ubuntu.Dockerfile (1)
Line range hint
1-25: LGTM! Consider further optimizing the Dockerfile.The changes to the Dockerfile improve compatibility, process management, and image size optimization. Here's a summary of the improvements:
- Conditional yarn configuration for armv7l architecture in the base stage.
- Addition of tini for better process management in the prod stage.
- Enhanced cleanup steps to reduce image size.
- Adjusted COPY and ADD commands to ensure all necessary files are included.
Consider the following minor optimization:
-RUN apt-get update && apt-get install tini && apt-get clean -y && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends tini \ + && apt-get clean -y \ + && rm -rf /var/lib/apt/lists/*This change:
- Adds
-yflag to avoid interactive prompts.- Uses
--no-install-recommendsto minimize unnecessary package installations.- Improves readability by breaking the command into multiple lines.
These modifications can further reduce the image size and improve build consistency.
🧰 Tools
🪛 Hadolint
[error] 23-23: Use COPY instead of ADD for files and folders
(DL3020)
docker/stable-alpine.Dockerfile (1)
Line range hint
1-26: Summary: Dockerfile changes support TypeScript integration and improved testabilityThe modifications to the Dockerfile align well with the PR objectives of enhancing testability and supporting TypeScript. The addition of
npmand the use oftsxfor execution are consistent changes that should improve the development and testing process.To ensure the changes don't introduce any unforeseen issues, consider the following final verification steps:
- Build the Docker image and run a container to verify that the application starts and functions correctly.
- Compare the startup time and resource usage of the new image with the previous version.
- Run your test suite using this Docker image to ensure all tests pass in the new environment.
These steps will help validate that the Dockerfile changes support the intended improvements without introducing new problems.
src/app-simplefin/models/transaction.ts (2)
1-9: LGTM! Consider adding JSDoc comments for better documentation.The class declaration and properties are well-structured and properly typed. The comment referencing the source of the transaction structure is helpful.
To improve documentation, consider adding JSDoc comments for each property. This will provide better intellisense support and make the code more self-documenting. For example:
/** * Represents a financial transaction. */ class Transaction { /** Unique identifier for the transaction */ id: string; /** Date when the transaction was posted */ posted: Date; /** Amount of the transaction (positive for deposits) */ amount: string; /** Description of the transaction */ description: string; /** Optional date when the transaction occurred */ transacted_at?: Date; /** Indicates if the transaction is pending */ pending: boolean; /** Additional transaction data */ extra?: object; // ... }
11-34: LGTM! Consider adding input validation for improved robustness.The constructor correctly initializes all properties and handles the conversion of Unix timestamps to Date objects. The use of the nullish coalescing operator for the
pendingproperty is a good practice.To improve robustness, consider adding input validation for required fields and type checking. Here's an example of how you could enhance the constructor:
constructor(data: { id: string; posted: number; amount: string; description: string; transacted_at?: number; pending?: boolean; extra?: object; }) { if (!data.id || typeof data.id !== 'string') { throw new Error('Invalid or missing id'); } if (typeof data.posted !== 'number') { throw new Error('Invalid posted date'); } if (!data.amount || typeof data.amount !== 'string') { throw new Error('Invalid or missing amount'); } if (!data.description || typeof data.description !== 'string') { throw new Error('Invalid or missing description'); } this.id = data.id; this.posted = new Date(data.posted * 1000); this.amount = data.amount; this.description = data.description; this.transacted_at = data.transacted_at ? new Date(data.transacted_at * 1000) : undefined; this.pending = data.pending ?? false; this.extra = data.extra; }This will ensure that the required fields are present and of the correct type before initializing the object.
docker/edge-ubuntu.Dockerfile (1)
Line range hint
23-25: Well-implemented user creation and data directory setupThe user and group creation commands make excellent use of the new ARG variables. Creating the
/datadirectory and setting its ownership to the new user is a good practice for data persistence and security.Consider adding a comment explaining the purpose of the
/datadirectory for better clarity:RUN groupadd --gid $USER_GID $USERNAME \ && useradd --uid $USER_UID --gid $USER_GID -m $USERNAME +# Create and set ownership of /data directory for application data persistence RUN mkdir /data && chown -R ${USERNAME}:${USERNAME} /datasrc/app-simplefin/tests/integration/app-simplefin.test.js (4)
3-15: LGTM:/statusendpoint test is well-structured.The test case for the
/statusendpoint is correctly implemented and checks for both the status code and the expected response body. This ensures that the endpoint behaves as expected when the application is not configured.Consider adding more test cases to cover different scenarios, such as:
- When the application is configured (if applicable).
- Error handling for unexpected server errors.
This will improve the test coverage and ensure robustness of the endpoint.
17-33: LGTM:/accountsendpoint test is comprehensive.The test case for the
/accountsendpoint is well-implemented, checking both the status code and the detailed error response for an invalid access token.To enhance the test suite:
- Consider adding a test case for a successful response (when a valid token is provided).
- Test the endpoint with various invalid input scenarios (e.g., malformed request body).
- Verify that the endpoint handles different HTTP methods correctly (e.g., GET, PUT) if applicable.
These additions would provide more comprehensive coverage of the endpoint's behavior.
35-53: LGTM:/transactionsendpoint test is well-implemented with good request body handling.The test case for the
/transactionsendpoint is correctly implemented, including a request body and checking both the status code and the detailed error response for an invalid access token.Consider the following enhancements:
- Add test cases for various date ranges and account IDs to ensure proper handling of different inputs.
- Include a test for a successful response scenario (with a valid token).
- Test error cases such as missing required fields or invalid date formats in the request body.
Note that all three endpoints return a 200 status code even for error responses. While this is consistent across the application, it's generally recommended to use appropriate HTTP status codes (e.g., 401 for unauthorized access, 400 for bad requests) to follow RESTful API best practices. Consider discussing with the team if this design decision should be revisited in the future.
1-53: Overall, good foundation for integration tests with room for expansion.The integration tests for the
/status,/accounts, and/transactionsendpoints provide a solid starting point for ensuring the correct behavior of the SimpleFIN application. The tests are well-structured, consistent, and cover the critical invalid token scenario.To further improve the test suite:
Expand test coverage:
- Add tests for successful scenarios (with valid tokens).
- Include edge cases and error scenarios (e.g., malformed requests, missing fields).
- Consider testing rate limiting, if applicable.
Refactor for reusability:
- Extract common assertions and request setups into helper functions to reduce code duplication.
Consider end-to-end testing:
- While these integration tests are valuable, also plan for end-to-end tests that cover the entire user flow, including token acquisition and refresh processes.
Review error handling strategy:
- Discuss the use of 200 status codes for error responses. Consider adopting standard HTTP status codes for different scenarios to align with RESTful API best practices.
Performance testing:
- As the application matures, consider adding performance tests to ensure the API can handle expected loads.
These improvements will enhance the robustness and reliability of your test suite, contributing to the overall quality of the SimpleFIN application.
src/util/camelCase.test.ts (2)
46-58: LGTM: Good test for non-recursive behavior, but consider adding a recursive test.This test case correctly verifies that the function doesn't recursively transform keys in nested objects, which is an important behavior to confirm.
Consider adding another test case to verify the function's behavior if recursive transformation is desired. This would involve creating a new test case with a nested object and expecting all levels of keys to be transformed. For example:
it('should recursively transform keys in nested objects', () => { const input = { 'outer-key': { 'inner-key': { 'nested-key': 'value' } } }; const expectedOutput = { outerKey: { innerKey: { nestedKey: 'value' } } }; expect(transformKeys(input, true)).toEqual(expectedOutput); });This assumes that the
transformKeysfunction could accept an optional boolean parameter to enable recursive transformation. If this functionality is needed, you might want to implement it in thetransformKeysfunction and add this test case.
1-59: Great job on the comprehensive test suite!The test cases in this file provide excellent coverage for the
transformKeysfunction, including various scenarios and edge cases. The tests are well-structured, clear, and follow good testing practices.To further enhance the test suite, consider the following suggestions:
- Add a test case for recursive transformation of nested objects, if that functionality is needed (as mentioned in the previous comment).
- Include a test case with a larger, more complex object to ensure the function performs well with a higher volume of keys.
- Consider adding a test case with non-string values (e.g., numbers, booleans, null) to ensure the function handles these correctly.
These additions would make the test suite even more robust and comprehensive.
src/app-simplefin/models/organization.test.ts (2)
1-1: LGTM! Consider omitting the file extension.The import statement is correct. However, in TypeScript projects, it's common to omit the '.ts' extension when importing local files. This can make your imports more resilient to potential future file extensions changes.
Consider modifying the import statement as follows:
-import Organization from './organization.ts'; +import Organization from './organization';
1-53: Great job on the comprehensive test suite! Consider adding an error handling test.The test suite for the
Organizationclass is well-structured, covering various scenarios including full JSON parsing, partial data handling, and direct object instantiation. The tests are clear, concise, and follow good practices.To further improve the robustness of your tests, consider adding a test case for error handling. This could include scenarios such as:
- Passing an invalid JSON string to
fromJson.- Creating an instance with missing required fields.
- Providing invalid data types for the fields.
Here's an example of how you might add an error handling test:
it('should throw an error when creating an Organization instance with invalid JSON', () => { const invalidJson = `{ "sfin-url": 123 // Invalid type for sfinUrl }`; expect(() => Organization.fromJson(invalidJson)).toThrow(); });This will help ensure that your
Organizationclass gracefully handles invalid input.src/app-simplefin/models/transaction.test.ts (3)
4-25: Good test coverage, consider addressing commented-out assertion.The test case thoroughly checks the creation of a Transaction instance from JSON, validating all main properties. The conversion of the
postedtimestamp to milliseconds ensures accurate comparison.Consider addressing the commented-out assertion for
extra.category:// expect(transactionFromJson.extra.category).toBe("food");If this property is intended to be part of the Transaction model, uncomment and implement the necessary logic. If not, consider removing the commented code to maintain cleanliness.
27-49: Good test coverage, maintain consistency with commented-out assertion.This test case effectively validates the creation of a Transaction instance from an object, covering all main properties. It maintains consistency with the first test case, which is excellent.
As with the previous test, consider addressing the commented-out assertion for
extra.category:// expect(transactionFromObject.extra.category).toBe("food");Ensure that the decision to implement or remove this assertion is consistent with the first test case.
1-49: Well-structured tests with good coverage, consider adding edge cases.The test file provides comprehensive coverage for the basic functionality of the Transaction class, testing both creation from JSON and object. The structure is clear and easy to follow.
To further improve the test suite, consider adding the following:
- Edge cases:
- Test with missing or null properties
- Test with invalid data types for properties
- Error scenarios:
- Test with malformed JSON
- Test with invalid date formats
- Boundary value tests:
- Test with very large or small amounts
- Test with extreme date values
These additional tests will help ensure the robustness of the Transaction class under various conditions.
package.json (1)
19-20: Approve test script improvements with a suggestionThe updates to the test scripts are beneficial:
- Suppressing warnings with
NODE_NO_WARNINGS=1cleans up the test output.- Adding
--experimental-vm-modulessupports ECMAScript modules, which is great for TypeScript usage.- The new coverage script enhances testing capabilities.
Suggestion: Consider moving the common Node options and environment variables to a separate configuration file or environment variables to reduce duplication between the
testandtest:coveragescripts.src/app-simplefin/models/account.test.ts (5)
1-3: Consider removing file extensions from import statements.While the current import statements work, it's generally recommended to omit the file extension (
.ts) when importing TypeScript files. This approach is more common and can prevent potential issues with module resolution in different environments.Consider updating the import statements as follows:
-import Account from './account.ts'; -import Organization from './organization.ts'; -import Transaction from './transaction.ts'; +import Account from './account'; +import Organization from './organization'; +import Transaction from './transaction';
6-51: Excellent test coverage for Account creation from full JSON.This test case thoroughly checks the creation of an Account instance from a JSON string containing all fields. It verifies the correct instantiation of nested objects, proper parsing of all fields, and handling of arrays.
For improved readability, consider extracting the JSON string to a separate constant:
const fullAccountJson = `{ "org": { "domain": "mybank.com", "sfin-url": "https://sfin.mybank.com" }, // ... rest of the JSON ... }`; const account = Account.fromJson(fullAccountJson);This separation would make the test structure clearer and easier to maintain.
53-80: Well-structured test for Account creation with minimal JSON.This test case effectively verifies the creation of an Account instance from JSON with only required fields. It properly checks that optional fields are undefined and that collections are empty when not provided.
For consistency with the previous test and improved readability, consider extracting the JSON string to a separate constant:
const minimalAccountJson = `{ "org": { "domain": "mybank.com", "sfin-url": "https://sfin.mybank.com" }, // ... rest of the JSON ... }`; const account = Account.fromJson(minimalAccountJson);This change would align the structure with the previous test case and improve overall readability.
82-124: Comprehensive test for direct Account instantiation.This test case effectively verifies the creation of an Account instance directly from an object literal. It properly handles nested object creation and thoroughly checks all properties of the resulting Account instance.
To improve code organization and readability, consider extracting the test data creation into a separate function:
function createTestAccountData() { const org = new Organization({ domain: 'mybank.com', sfinUrl: 'https://sfin.mybank.com', }); const transaction = new Transaction({ id: '12394832938403', posted: 793090572, amount: '-33293.43', description: "Uncle Frank's Bait Shop", }); return { org, id: '2930002', name: 'Savings', currency: 'USD', balance: '100.23', availableBalance: '75.23', balanceDate: 978366153, transactions: [transaction], extra: { accountOpenDate: 978360153, }, }; } // In the test: const data = createTestAccountData(); const account = new Account(data);This refactoring would separate the test data creation from the actual test logic, making the test more readable and easier to maintain.
1-125: Excellent test coverage for the Account class.This test file provides comprehensive coverage for the Account class, including tests for creation from full JSON, minimal JSON, and direct object instantiation. The tests are well-structured, use appropriate assertions, and cover all major scenarios.
For future improvements, consider:
- Adding edge case tests, such as handling of invalid input or boundary conditions.
- Implementing parameterized tests for different account types or currencies, if applicable.
- Adding tests for any methods that might be added to the Account class in the future.
These additions would further enhance the robustness of your test suite and ensure continued reliability as the Account class evolves.
src/app-simplefin/models/account-set.test.ts (3)
1-3: Consider removing file extensions from import statements.TypeScript typically doesn't require file extensions in import statements. Removing them can make the code more maintainable if file extensions change in the future.
Consider updating the import statements as follows:
-import AccountSet from './account-set.ts'; -import Account from './account.ts'; -import Transaction from './transaction.ts'; +import AccountSet from './account-set'; +import Account from './account'; +import Transaction from './transaction';
6-61: Excellent test coverage for AccountSet with all fields.This test case thoroughly checks the creation of an AccountSet instance with all fields populated, including nested structures. The assertions are comprehensive and well-structured.
To improve readability, consider extracting the JSON string into a separate constant:
const fullAccountSetJson = `{ "errors": [], "accounts": [ { // ... (rest of the JSON) } ] }`; const accountSet = AccountSet.fromJson(fullAccountSetJson);This separation would make the test setup more clear and easier to maintain.
110-134: Consider enhancing the test case for empty errors array.While this test case covers the scenario of an empty errors array, it could be more comprehensive. The current assertions are minimal and don't fully validate the account details.
Consider expanding the assertions to check the account properties, similar to the previous test cases. For example:
expect(accountResponse.accounts[0].id).toBe("2930002"); expect(accountResponse.accounts[0].name).toBe("Savings"); expect(accountResponse.accounts[0].currency).toBe("USD"); expect(accountResponse.accounts[0].balance).toBe("100.23"); expect(accountResponse.accounts[0].balanceDate).toEqual(new Date(978366153 * 1000));This would provide more thorough validation of the AccountSet creation in this scenario.
src/load-config.js (3)
75-75: LGTM! Consider adding documentation for the new config option.The addition of the
useRequestLoggerproperty to thedefaultConfigobject is a good enhancement for configuration management. It aligns well with the PR objectives of improving testability.Consider adding a comment explaining the purpose and impact of this new configuration option. This will help other developers understand its usage and implications.
87-87: LGTM! Consider adding a comment for clarity.Setting
useRequestLoggertofalsein the test environment is a good practice. It aligns with the PR objectives of cleaning up testing output and enhancing testability.For improved clarity, consider adding a brief comment explaining why request logging is disabled in the test environment. For example:
// Disable request logging in test environment to reduce noise in test outputs useRequestLogger: false,
Line range hint
1-161: Summary of changes and recommendationsThe introduction of the
useRequestLoggerconfiguration option is a positive change that aligns well with the PR objectives of enhancing testability and configuration management. Here's a summary of the review:
- The
useRequestLoggerproperty is correctly added to thedefaultConfigobject.- The property is appropriately set to
falsein the test environment configuration.- There's a potential issue where
useRequestLoggeris not included in thefinalConfigobject, which needs to be addressed.Overall, the changes are good, but please ensure you address the
finalConfigissue to fully implement this new feature. Also, consider adding comments to improve code clarity as suggested in the previous review comments.As you continue to enhance the configuration management, consider creating a separate configuration schema or type definition file. This would help enforce type safety and provide better documentation for all configuration options. It could be particularly useful as you add more configuration options in the future.
src/app-simplefin/tests/unit/simplefin-api.test.ts (4)
9-98: LGTM: Comprehensive test suite for SimplefinContextData.The test suite covers various methods and scenarios for the SimplefinContextData class, including initialization, parsing access keys, building headers, and URL construction. The tests are well-defined and cover both normal and error cases.
Consider adding a test case for the
normalizeDatemethod with an edge case, such as a date at the year boundary or a leap year, to ensure robust date handling.
100-122: LGTM: FakeHttpClient implementation is suitable for testing.The FakeHttpClient class provides a good mock implementation of the HttpClient interface, allowing for setting predefined responses and tracking requests. This is very useful for verifying API calls in the tests.
Consider adding a method to clear the responses and requests between tests, which could help ensure test isolation:
clearState() { this.responses = {}; this.requests = []; }This method could be called in the
beforeEachblock of the test suite to ensure a clean state for each test.
146-260: LGTM: Comprehensive test cases for SimplefinApi.The test suite covers the main functionality of the SimplefinApi class, including setting context, fetching accounts, fetching access keys, and error handling. The use of FakeHttpClient for mocking API responses is well implemented, and the assertions check both returned data and request structure.
Consider the following improvements:
Add a test case for handling an empty response from the API to ensure proper behavior in such scenarios.
Implement a test for concurrent API calls to verify thread safety, if applicable to your use case.
Consider parameterizing some tests, especially for error handling, to cover a wider range of scenarios without duplicating code. For example:
const errorScenarios = [ { name: 'Network Error', error: new Error('Network Error') }, { name: 'Timeout', error: new Error('Request timed out') }, // Add more scenarios as needed ]; errorScenarios.forEach(({ name, error }) => { it(`should handle ${name} when fetching accounts`, async () => { // Test implementation }); });These additions would further enhance the robustness of your test suite.
1-260: Overall, excellent test implementation with room for minor enhancements.The unit tests for SimplefinContextData and SimplefinApi classes are comprehensive and well-structured. The use of FakeHttpClient for mocking HTTP requests is a good approach for isolating the tests from external dependencies.
To further improve the test suite:
Consider implementing property-based testing for functions that handle various input types, such as the
normalizeDatemethod. This can help uncover edge cases that might be missed with manual test case selection.As the codebase grows, consider organizing the tests into separate files for each class (SimplefinContextData and SimplefinApi) to improve maintainability.
If not already in place, consider setting up test coverage reporting to identify any gaps in the current test suite and maintain high coverage as the codebase evolves.
These suggestions aim to enhance the long-term maintainability and robustness of your test suite.
src/app-sync.js (1)
184-187: Approve changes with minor suggestions for improvementThe addition of the group sync data cleanup is a good practice. It ensures that unnecessary data is removed when a user file is reset. Here are a few suggestions to enhance this implementation:
- Consider logging the actual error object for more detailed debugging information:
} catch (e) { console.log(`Unable to delete sync data for group "${group_id}":`, e); }
- You might want to consider more robust error handling, such as notifying the user if the cleanup fails. This could be done by adding a warning to the response:
let warning = null; try { await fs.unlink(getPathForGroupFile(group_id)); } catch (e) { console.log(`Unable to delete sync data for group "${group_id}":`, e); warning = 'Failed to delete old sync data. This won\'t affect your current data, but you may want to manually clean up old files.'; } res.send({ ...OK_RESPONSE, warning });These changes would provide more detailed logging for debugging and give users more information about the process.
src/app-simplefin/httpClient.ts (1)
16-16: Simplify by passing the URL string directly tohttps.request.Passing the URL string directly to
https.requestis sufficient and avoids the need to create aURLobject explicitly.Modify the request initialization:
- const req = https.request(new URL(url), options, (response) => { + const req = https.request(url, options, (response) => {This makes the code cleaner and leverages
https.request's ability to parse the URL internally.src/app-simplefin/models/account.ts (2)
1-3: Consider omitting the.tsextensions in import statements.In TypeScript, it's common practice to omit the
.tsfile extensions in import paths. Including the extension may cause issues with module resolution depending on your build configuration.Apply this change:
-import Organization from './organization.ts'; -import Transaction from './transaction.ts'; -import { transformKeys } from '../../util/camelCase.ts'; +import Organization from './organization'; +import Transaction from './transaction'; +import { transformKeys } from '../../util/camelCase';
13-14: Clarify the sorting criteria for transactions.The comment mentions that transactions are ordered by "posted." To enhance clarity, specify the exact property used for ordering, such as
postedDateortimestamp. This will help maintainers understand the sorting logic.src/app-simplefin/services/simplefin-api.ts (4)
51-68: Consider makingpendingparameter configurableIn the
buildAccountQueryStringmethod, thependingparameter is hardcoded to1. Depending on the API requirements, it might be beneficial to allow this parameter to be configurable.Consider adding a parameter to the method:
- buildAccountQueryString(startDate: Date, endDate: Date) { + buildAccountQueryString(startDate: Date, endDate: Date, includePending: boolean = true) { const params = []; let queryString = ''; if (startDate) { params.push(`start-date=${this.normalizeDate(startDate)}`); } if (endDate) { params.push(`end-date=${this.normalizeDate(endDate)}`); } - params.push(`pending=1`); + if (includePending) { + params.push(`pending=1`); + } if (params.length > 0) { queryString += '?' + params.join('&'); } this.queryString = queryString; }
91-93: Simplify date normalizationThe
normalizeDatemethod can be simplified by usingMath.flooron the timestamp, which may improve readability.Apply this diff:
- return Date.parse(date.toISOString()) / 1000; + return Math.floor(date.getTime() / 1000);
70-79: Remove redundant checks inaccountsUrlThe check for
params.length > 0in thebuildAccountQueryStringmethod ensuresthis.queryStringis always defined whenaccountsUrlis called. The additional check forthis.queryStringbeingundefinedmight be unnecessary.Consider removing the redundant check:
- if (this.queryString === undefined) { - throw new GenericSimplefinError('Query string must be defined'); - }
139-139: Export interfaces before classesFor better readability and to follow TypeScript conventions, consider exporting interfaces before classes.
-export { SimplefinApi, SimplefinContextData, SimpleFinApiInterface }; +export { SimpleFinApiInterface, SimplefinContextData, SimplefinApi };src/app-simplefin/app-simplefin.js (1)
191-191: Address the TODO: Determine the Usage of 'trans.payee'There is a TODO comment questioning the usage of
trans.payee. Since thepayeefield may not be present in the SimpleFIN API response, assigning it tonewTrans.payeeNamemight result inundefined.Would you like assistance in verifying whether
trans.payeeis available in the API response? I can help update the code to handle this field appropriately or remove it if it's not used. Let me know if you'd like me to open a new GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
upcoming-release-notes/440.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
- docker/edge-alpine.Dockerfile (2 hunks)
- docker/edge-ubuntu.Dockerfile (1 hunks)
- docker/stable-alpine.Dockerfile (2 hunks)
- docker/stable-ubuntu.Dockerfile (1 hunks)
- jest.config.json (1 hunks)
- package.json (3 hunks)
- src/app-simplefin/app-simplefin.js (6 hunks)
- src/app-simplefin/errors.ts (1 hunks)
- src/app-simplefin/httpClient.ts (1 hunks)
- src/app-simplefin/models/account-set.test.ts (1 hunks)
- src/app-simplefin/models/account-set.ts (1 hunks)
- src/app-simplefin/models/account.test.ts (1 hunks)
- src/app-simplefin/models/account.ts (1 hunks)
- src/app-simplefin/models/organization.test.ts (1 hunks)
- src/app-simplefin/models/organization.ts (1 hunks)
- src/app-simplefin/models/transaction.test.ts (1 hunks)
- src/app-simplefin/models/transaction.ts (1 hunks)
- src/app-simplefin/services/simplefin-api.ts (1 hunks)
- src/app-simplefin/services/simplefin-service.ts (1 hunks)
- src/app-simplefin/tests/integration/app-simplefin.test.js (1 hunks)
- src/app-simplefin/tests/unit/simplefin-api.test.ts (1 hunks)
- src/app-simplefin/tests/unit/simplefin-service.test.ts (1 hunks)
- src/app-sync.js (1 hunks)
- src/app-sync.test.js (1 hunks)
- src/config-types.ts (1 hunks)
- src/load-config.js (2 hunks)
- src/services/secrets-service.js (1 hunks)
- src/util/camelCase.test.ts (1 hunks)
- src/util/camelCase.ts (1 hunks)
- src/util/middlewares.js (2 hunks)
- tsconfig.json (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/services/secrets-service.js
🧰 Additional context used
🔇 Additional comments (57)
src/util/camelCase.ts (1)
18-18: Export statement looks good.The export statement correctly exports only the
transformKeysfunction, which is likely the main utility needed by other modules. This approach maintains good encapsulation by keeping the helper functions (isKebabCaseandkebabToCamel) private to this module.src/config-types.ts (1)
23-23: Approved, but clarification needed on PR objectives.The addition of the
useRequestLoggerproperty to theConfiginterface is straightforward and doesn't introduce any issues. This boolean flag will allow for configurable request logging, which can be useful for debugging and monitoring.However, I noticed that this change doesn't directly align with the stated PR objectives of enhancing testability and introducing new components like
HTTPSClient,SimplefinAPI, andSimplefinService.Could you please clarify how this configuration option relates to the overall objectives of the PR? Is it part of a larger refactoring effort not explicitly mentioned in the PR summary?
To verify the usage of this new property, let's run the following script:
This will help us understand how the new property is being utilized across the project.
jest.config.json (2)
12-12: LGTM: Treating TypeScript files as ECMAScript modules.Adding
"extensionsToTreatAsEsm": [".ts"]is a good change that aligns with the PR objective of enabling Jest to test TypeScript files directly. This should improve the testing process for TypeScript code.
15-15: Consider the trade-offs of silent test execution.Setting
"silent": truewill clean up the testing output, which aligns with the PR objectives. However, be aware that this might hide useful debugging information during test runs.To ensure this doesn't cause issues:
- Verify that there's a way to enable verbose output when needed (e.g., for debugging).
- Check if any existing tests rely on console output for verification.
#!/bin/bash # Check for tests that might rely on console output rg --type typescript --type javascript 'console\.(log|error|warn|info)' src/__tests__src/app-simplefin/models/account-set.ts (3)
8-11: LGTM! Constructor implementation is correct and type-safe.The constructor correctly initializes the class properties using the provided
dataparameter. The parameter type matches the property types, ensuring type safety.
22-22: LGTM! Export statement is correct.The
AccountSetclass is correctly exported as the default export, following common JavaScript/TypeScript practices.
1-22: Overall, theAccountSetclass is well-implemented and aligns with the PR objectives.The introduction of this class contributes to the goal of refactoring the SimpleFin integration for better testability. The structure allows for easy unit testing of
AccountSetfunctionality. The implementation follows TypeScript best practices and provides a clear representation of the SimpleFin Account Set concept.Minor improvements have been suggested for:
- Import statement formatting
- Enhanced documentation
- Robust error handling in the
fromJsonmethodThese changes will further improve the code quality and reliability of the
AccountSetclass.tsconfig.json (4)
23-23: LGTM: Include all TypeScript files.The new "include" property ensures that all TypeScript files in the project are included in the compilation process. This change aligns well with the PR objective of enhancing TypeScript usage.
24-24: LGTM: Appropriate exclusions.The updated "exclude" property correctly excludes common directories that don't need to be processed by the TypeScript compiler. This change helps improve compilation speed and avoids unnecessary processing.
25-28: ts-node configuration looks good, but be aware of experimental option.The new ts-node configuration enables ESM support and Node.js-like module resolution, which aligns with the PR objectives. However, please note:
- The
experimentalSpecifierResolutionoption is marked as experimental. While it's useful for your current needs, be prepared to update it in the future if it changes or becomes deprecated.To verify the ts-node configuration, run the following script:
#!/bin/bash # Description: Verify ts-node can run TypeScript files directly # Test: Create a simple TypeScript file and run it with ts-node echo "console.log('Hello from TypeScript!');" > test.ts npx ts-node test.ts # Clean up rm test.ts # Check if any TypeScript files use ESM syntax if grep -R "import \|export \|import(" --include="*.ts" .; then echo "ESM syntax found in TypeScript files. The new configuration should support this." else echo "No ESM syntax found in TypeScript files. Consider if the ESM configuration is necessary." fi
7-9: New compiler options look good, but verify build process.The new compiler options enhance TypeScript support and align with the PR objectives. However, please ensure that:
- The
allowImportingTsExtensionsoption doesn't cause issues in production builds.- The
emitDeclarationOnlyoption doesn't interfere with your build process if you rely on compiled JS files.To verify the impact of these changes, run the following script:
src/app-simplefin/models/organization.ts (3)
1-9: LGTM: Class definition aligns with SimpleFIN protocolThe
Organizationclass is well-structured and follows the SimpleFIN protocol. The use of optional properties forname,domain,url, andidis appropriate, whilesfinUrlis correctly marked as required. The property names follow the camelCase convention, which is consistent with TypeScript best practices.
11-23: LGTM: Well-implemented constructorThe constructor is implemented correctly, using a single object parameter for all properties. This approach provides flexibility and makes it easy to add new properties in the future without breaking existing code. The type definition in the parameter ensures type safety during object creation.
36-36: LGTM: Appropriate use of default exportThe use of default export for the
Organizationclass is appropriate and follows common TypeScript/JavaScript module patterns. This allows for clean and straightforward importing in other files.docker/stable-ubuntu.Dockerfile (1)
26-26: LGTM! Verify TypeScript execution in the container environment.The change from
CMD ["node", "app.js"]toCMD ["npx", "tsx", "app.js"]aligns with the PR objective of enabling direct execution of TypeScript files. This modification allows running TypeScript files without a separate compilation step, which can simplify the development and deployment process.To ensure this change works as expected, please verify:
- The
tsxpackage is correctly installed in the container.- The application starts successfully with this new command in the containerized environment.
You can run the following commands to verify:
These commands will help confirm that the new execution method works correctly in the Docker environment.
docker/stable-alpine.Dockerfile (2)
11-11: LGTM: Addition of npm package installationThe addition of
npmto the package installation list is appropriate and aligns with the changes in the CMD instruction. This ensures thatnpx(which comes with npm) will be available for executing the application.
26-26: LGTM: Updated execution method to support TypeScriptThe change from
node app.jstonpx tsx app.jsis appropriate and aligns with the PR objectives. This modification allows for direct execution of TypeScript files, which enhances the development and testing process.Benefits:
- Enables TypeScript support without the need for a separate compilation step.
- Improves consistency between development and production environments.
- Facilitates easier testing of TypeScript files.
To ensure this change doesn't introduce any runtime issues, please verify the following:
- The application starts correctly with this new command.
- There's no significant impact on startup time.
- All functionality works as expected when running with
tsx.You can use the following script to check if
app.js(orapp.ts) uses any Node.js-specific APIs that might not be compatible withtsx:If the script returns any matches, review those lines to ensure compatibility with
tsx.✅ Verification successful
Verified: Updated execution method is compatible
The change from
node app.jstonpx tsx app.jsdoes not introduce any Node.js-specific APIs that are incompatible withtsx. The verification confirms that the application can run as expected without runtime issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Node.js-specific APIs that might not be compatible with tsx rg --type typescript --type javascript '(require\(|module\.exports|__dirname|__filename)' app.js app.tsLength of output: 140
Script:
#!/bin/bash # Check for Node.js-specific APIs that might not be compatible with tsx in .js and .ts files rg '(require\(|module\.exports|__dirname|__filename)' --glob '*.js' --glob '*.ts'Length of output: 83
src/app-simplefin/models/transaction.ts (2)
41-46: LGTM! TheisPendingmethod is well-implemented.The
isPendingmethod correctly handles both explicit and implicit pending states. It first checks thependingproperty, and if it's undefined, it determines the pending status based on whether theposteddate is set to zero. This approach is logical and covers all scenarios.
49-49: LGTM! Default export is appropriate for this module.Exporting the Transaction class as the default export is a good choice for this module. It allows for clean and straightforward importing in other files where this class is needed.
src/util/middlewares.js (2)
2-2: LGTM: New import statement added.The new import for
configis correctly added and will be used in therequestLoggerMiddlewareconfiguration.
Line range hint
1-48: Summary: Improved logging configurationThe changes in this file focus on making the request logging more configurable by utilizing the
config.useRequestLoggersetting. This aligns well with the PR's objective of improving testability and allows for more flexible logging control across different environments. The core functionality of the middlewares remains unchanged, maintaining the existing error handling and user validation logic.docker/edge-ubuntu.Dockerfile (3)
Line range hint
20-22: Excellent addition of ARG variables for user creationThe introduction of
USERNAME,USER_UID, andUSER_GIDas ARG variables is a great improvement. This change allows for more flexibility in user creation and aligns with best practices for running containers with non-root users. The default values are reasonable, and using$USER_UIDforUSER_GIDensures consistency.
36-36: Approved: Updated CMD to use tsx for TypeScript executionThe change from
CMD ["node", "app.js"]toCMD ["npx", "tsx", "app.js"]is a good improvement. This allows direct execution of TypeScript files without a separate compilation step, aligning with the PR objectives.To ensure this change works as expected, please verify that
tsxis installed in the production image. You can add a RUN command in the Dockerfile to check this:ENV ACTUAL_WEB_ROOT=/public EXPOSE 5006 +RUN npx tsx --version || (echo "tsx is not installed" && exit 1) CMD ["npx", "tsx", "app.js"]This will fail the build if
tsxis not available, ensuring the new CMD will work correctly.
Line range hint
1-36: Overall Dockerfile improvements enhance security and TypeScript supportThe changes made to this Dockerfile are well-thought-out and align with the PR objectives. Key improvements include:
- Introduction of user-related ARG variables for better flexibility and security.
- Proper setup of a non-root user and data directory.
- Updated CMD to support direct execution of TypeScript files.
These changes collectively enhance the container's security posture and streamline the development process by enabling direct TypeScript execution. The Dockerfile now better supports the goal of making the application more easily testable and maintainable.
docker/edge-alpine.Dockerfile (3)
19-19: Improved package installation for production stageThe package installation has been optimized for the production environment. The explicit inclusion of
npmaligns with the new CMD instruction usingnpx. Addingtiniis a good practice for proper init process handling in containers. These changes contribute to a more streamlined and maintainable Docker image.
Line range hint
1-36: Overall Dockerfile changes and clarification neededThe Dockerfile changes generally align with the PR objectives of refactoring and improving testability. The shift to npm and TypeScript execution is consistent with these goals. However, there are a couple of points that require attention:
Desktop Client Download: A new step has been added to download and unzip a desktop client from GitHub. This addition is not mentioned in the PR objectives and might need clarification. Could you please explain the purpose of this change and how it relates to the refactoring effort?
Security Consideration: The use of a GitHub token for downloading artifacts could have security implications. Ensure that this token has the minimum necessary permissions and consider using Docker secrets or build arguments to handle sensitive information more securely.
To verify the desktop client download step, please run the following script:
#!/bin/bash # Verify desktop client download step # Check if the GITHUB_TOKEN is properly used as a build argument echo "Checking Dockerfile for GITHUB_TOKEN usage:" grep -n "ARG GITHUB_TOKEN" docker/edge-alpine.Dockerfile # Verify the curl command for downloading the desktop client echo "Checking curl command for desktop client download:" grep -n "curl -L -o /tmp/desktop-client.zip" docker/edge-alpine.Dockerfile # Check if the downloaded client is properly unzipped to /public echo "Checking unzip command for desktop client:" grep -n "unzip /tmp/desktop-client.zip -d /public" docker/edge-alpine.Dockerfile
36-36: Updated CMD instruction for TypeScript executionThe change to use
npx tsx app.jsinstead ofnode app.jsallows for direct execution of TypeScript files, which aligns with the refactoring objectives and can improve development workflow. This change is approved, but please consider the following points:
- Performance: Using
tsxfor runtime transpilation might have a slight performance impact compared to pre-compiled JavaScript. Ensure this doesn't affect the application's responsiveness in production.- Deployment: Verify that all necessary TypeScript files are included in the Docker image and that the
tsxpackage is correctly installed as a dependency.- Error Handling: Ensure that runtime TypeScript errors are properly logged and don't cause unexpected application behavior.
To verify the TypeScript setup, please run the following script:
✅ Verification successful
CMD instruction change verified and approved
The shift to
npx tsx app.jsfromnode app.jsis confirmed to be supported by the existing TypeScript setup. The TypeScript configuration and dependencies are correctly in place, ensuring smooth execution.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify TypeScript setup # Check if tsx is installed as a dependency echo "Checking for tsx in package.json:" jq '.dependencies.tsx' package.json # Verify that all necessary TypeScript files are present echo "Listing TypeScript files in src directory:" fd -e ts -e tsx . src # Check for any potential TypeScript configuration echo "Checking for tsconfig.json:" cat tsconfig.json 2>/dev/null || echo "tsconfig.json not found"Length of output: 736
Script:
#!/bin/bash # Verify TypeScript setup # Check if tsx is installed as a dependency or devDependency echo "Checking for tsx in package.json dependencies:" jq '.dependencies.tsx // .devDependencies.tsx' package.json # Verify that all necessary TypeScript files are present echo "Listing TypeScript files in src directory:" fd -e ts -e tsx . src # Check for the existence of tsconfig.json echo "Checking for tsconfig.json:" if [ -f tsconfig.json ]; then echo "tsconfig.json exists." else echo "tsconfig.json not found." fiLength of output: 1225
Script:
#!/bin/bash # Verify TypeScript setup with detailed debugging echo "Starting TypeScript setup verification..." # Check if tsx is installed in dependencies echo "Checking for tsx in package.json dependencies:" jq '.dependencies.tsx // .devDependencies.tsx // "Not found"' package.json # Verify that all necessary TypeScript files are present echo "Listing TypeScript files in src directory:" fd --extension ts --extension tsx . src || echo "No TypeScript files found in src." # Check for the existence of tsconfig.json echo "Checking for tsconfig.json:" if [ -f tsconfig.json ]; then echo "tsconfig.json exists." else echo "tsconfig.json not found." fi echo "TypeScript setup verification completed."Length of output: 1442
src/app-simplefin/tests/integration/app-simplefin.test.js (1)
1-2: LGTM: Imports are correct and well-structured.The import statements are appropriate for the integration tests. Using
handlers as appprovides clarity about the imported module's purpose.src/util/camelCase.test.ts (5)
1-1: LGTM: Import statement is correct.The import statement correctly imports the
transformKeysfunction from the relative path './camelCase.ts'.
4-14: LGTM: Well-structured test case for basic functionality.This test case effectively covers the primary use case of transforming kebab-case keys to camelCase. The input and expected output are clearly defined, and the assertion uses the appropriate
toEqualmethod for deep object comparison.
16-26: LGTM: Comprehensive test for mixed key formats.This test case effectively verifies that the function only transforms kebab-case keys while leaving camelCase keys unchanged. It's an important edge case to cover, ensuring the function's behavior is correct in mixed scenarios.
28-38: LGTM: Proper edge case testing for all camelCase keys.This test case correctly verifies that the function leaves objects unchanged when all keys are already in camelCase. It's a crucial edge case to cover, ensuring the function doesn't unnecessarily modify objects.
40-44: LGTM: Proper boundary condition test for empty object input.This test case correctly verifies the function's behavior when given an empty object as input. It's an important boundary condition to test, ensuring the function handles edge cases properly.
src/app-simplefin/models/organization.test.ts (3)
4-20: LGTM! Comprehensive test case for full JSON object creation.This test case thoroughly checks the creation of an
Organizationinstance from a complete JSON string. It verifies both the instance type and all individual properties, ensuring thefromJsonmethod works as expected for a full set of data.
22-34: LGTM! Well-structured test for partial JSON object creation.This test case effectively verifies the
Organizationclass's behavior when instantiated with only the required field. It correctly checks that optional fields are undefined, demonstrating robust handling of partial data.
36-52: LGTM! Thorough test for direct object instantiation.This test case effectively verifies the direct creation of an
Organizationinstance from a JavaScript object. It comprehensively checks all properties, ensuring the constructor works correctly with a complete set of data.src/app-simplefin/models/transaction.test.ts (1)
1-1: LGTM: Import statement is correct.The import statement correctly imports the
Transactionclass from the local file. Including the '.ts' extension is fine and can help with clarity.package.json (6)
8-8: LGTM: Improved TypeScript support in start scriptThe change from
node apptotsx appenhances the development workflow by allowing direct execution of TypeScript files. This aligns well with the PR's objective of improving testability and TypeScript integration.
25-25: LGTM: Improved TypeScript declaration generationThe change from
--noEmitto--emitDeclarationOnlyin the types script is a good improvement. This allows for generating TypeScript declaration files (.d.ts) without emitting JavaScript, which is useful for type checking and IDE support while maintaining the existing build process.
47-47: LGTM: Addition of tsx for improved TypeScript executionThe addition of the
tsxpackage (version ^4.17.0) is a good choice. It enables direct execution of TypeScript files without a separate compilation step, which streamlines the development process and aligns well with the changes made to the start script.
Line range hint
1-75: Summary of package.json changesThe changes in this file generally improve the project's development and testing workflow, aligning well with the PR objectives. Key improvements include:
- Enhanced TypeScript support with
tsxfor direct execution.- Comprehensive Docker scripts for various environments.
- Improved test scripts with coverage reporting.
- Updates to TypeScript-related dependencies.
However, please address the following points:
- Ensure secure handling of the GitHub token in Docker build scripts.
- Verify the unusually high version numbers for TypeScript and related ESLint packages.
- Consider reducing duplication in test scripts by using a common configuration.
Overall, these changes represent a positive step towards better testability and TypeScript integration.
9-15: Approve Docker scripts with security considerationThe addition of Docker-related scripts enhances build and run capabilities, which is great for deployment and testing. However, there's a potential security concern:
- The use of
$(gh auth token)in thedocker:build-commonscript might expose the GitHub token if not handled carefully.Consider using a more secure method to pass the GitHub token, such as environment variables or secrets management tools.
To ensure the GitHub token is not exposed, run the following script:
#!/bin/bash # Check if the GitHub token is visible in Docker build logs yarn docker:build-common 2>&1 | grep -i "github.*token"If this script returns any output, it indicates that the token might be visible in the logs, which is a security risk.
69-69: Approve TypeScript update with version concernUpdating the TypeScript version is generally good for accessing new features and improvements. However, version
^5.5.4seems unusually high, as the latest stable versions are typically around 5.0.x or 5.1.x.Please verify if this version is correct or if it's a typo. If it's intentional, ensure that it's compatible with your other dependencies and doesn't introduce any breaking changes.
To check the latest available versions of TypeScript, run:
#!/bin/bash npm view typescript versions --json | jq '.[-5:]'This will show the latest 5 versions of TypeScript. Compare these with the version specified in the package.json.
src/app-simplefin/tests/unit/simplefin-service.test.ts (3)
1-10: LGTM: Imports and constants are well-defined.The imports are correctly structured, and the ACCESS_KEY constant is appropriately defined for use in the tests.
11-20: LGTM: FakeSimplefinAPI class structure is well-defined.The class correctly implements the SimpleFinApiInterface and provides the necessary methods for testing.
58-79: LGTM: Test suite structure and coverage are well-defined.The test suite is well-structured using Jest, and it covers the main functionalities of the SimpleFinService. The use of beforeEach for setup is a good practice.
src/app-simplefin/models/account-set.test.ts (3)
63-96: Well-structured test for AccountSet with only required fields.This test case effectively verifies the creation of an AccountSet instance with only the required fields. It correctly checks the handling of optional fields, ensuring they are either undefined or empty as expected.
The test provides good coverage for the minimal use case, complementing the previous test with all fields.
98-108: Good coverage of edge case with empty accounts array.This test case effectively verifies the behavior of AccountSet when created with an empty accounts array. It's important to test this edge case to ensure the class handles it correctly.
The assertions are concise and cover the necessary checks for this scenario.
1-134: Overall, excellent test coverage for AccountSet class.This test file provides comprehensive coverage for the AccountSet class, including various scenarios such as full data, minimal required fields, and edge cases. The use of Jest and the consistent structure of test cases contribute to the maintainability of the test suite.
To further improve the test suite, consider the following suggestions:
- Use constants for repeated values (e.g., account IDs, dates) to improve maintainability.
- Add a test case for handling invalid JSON input to ensure proper error handling.
- Consider using Jest's
beforeEachto set up common test data, reducing repetition across test cases.These enhancements would make the test suite even more robust and easier to maintain in the future.
src/load-config.js (1)
Line range hint
101-161: EnsureuseRequestLoggeris included in thefinalConfigThe new
useRequestLoggerproperty has been added to thedefaultConfigandconfigobjects, but it's not included in thefinalConfigobject that's exported at the end of the file. This might prevent other parts of the application from accessing this new configuration option.To resolve this, add the
useRequestLoggerproperty to thefinalConfigobject. You can do this by modifying thefinalConfigdeclaration as follows:const finalConfig = { ...config, useRequestLogger: process.env.ACTUAL_USE_REQUEST_LOGGER ? process.env.ACTUAL_USE_REQUEST_LOGGER.toLowerCase() === 'true' : config.useRequestLogger, // ... other existing properties };This change will ensure that the
useRequestLoggersetting can be overridden by an environment variable if needed, maintaining consistency with how other configuration options are handled.After making this change, please run the following command to verify that the
useRequestLoggerproperty is correctly included in the exported configuration:This should show the new property in both the
defaultConfigandfinalConfigobjects.src/app-simplefin/tests/unit/simplefin-api.test.ts (2)
1-8: LGTM: Imports and initial setup look good.The import statements are appropriate for the context of the file, including necessary dependencies and types from local files.
124-144: LGTM: Well-structured setup for SimplefinApi test suite.The test suite setup is comprehensive, initializing all necessary components and using a beforeEach block to ensure a clean state for each test. The use of mock data (simplefinBase64Token and simplefinAccessKey) provides consistency across tests.
src/app-simplefin/httpClient.ts (1)
34-34: Ensure support for request bodies in methods like POST or PUT.The current implementation does not handle sending a request body, which is necessary for methods like
POSTorPUT. If the client should support these methods, you'll need to handle writing data to the request.Please confirm if sending a request body is required. If so, modify the
requestmethod to accept a request body and write it before ending the request.export interface HttpClient { - request(url: string, options: CustomRequestOptions): Promise<string>; + request(url: string, options: CustomRequestOptions, body?: string): Promise<string>; } export class HttpsClient implements HttpClient { - request(url: string, options: CustomRequestOptions): Promise<string> { + request(url: string, options: CustomRequestOptions, body?: string): Promise<string> { // ... req.end( + body ); } }src/app-simplefin/models/account.ts (2)
34-34: Verify the correctness ofbalanceDatetimestamp conversion.In the constructor,
balanceDateis calculated usingnew Date(data.balanceDate * 1000), which assumesdata.balanceDateis a Unix timestamp in seconds. Please ensure that thebalance-datefield in the JSON is indeed in seconds. If it's already in milliseconds, multiplying by 1000 would result in an incorrect date.Also applies to: 55-55
4-63: TheAccountclass implementation looks solid.The class accurately represents an account with all the necessary properties and methods. The use of
fromJsonfor object creation and handling of optional fields is appropriate.src/app-simplefin/app-simplefin.js (2)
178-183: Confirm 'trans.transacted_at' and 'trans.posted' Are Valid DatesIn the conditional statements on lines 178-183,
trans.transacted_atandtrans.postedare used to obtain timestamps. Ensure that these properties are always validDateobjects to prevent runtime errors when callinggetTime().To confirm that
trans.transacted_atandtrans.postedare always validDateobjects, you can run the following script:#!/bin/bash # Description: Verify that 'transacted_at' and 'posted' are Date objects in all Transaction instances. # Test: Search for the Transaction class definition and check the types of 'transacted_at' and 'posted'. ast-grep --lang typescript --pattern ' class Transaction { $$$ transacted_at: Date posted: Date $$$ } 'This script uses
ast-grepto find theTransactionclass and ensure that the properties are typed asDate.
147-147:⚠️ Potential issueEnsure 'account.balanceDate' is Defined Before Use
On line 147,
account.balanceDateis used without checking if it is defined. Ifaccount.balanceDateisnullorundefined, callingtoISOString()will throw an error.Consider adding a check to ensure
account.balanceDateis valid:- const date = account.balanceDate.toISOString().split('T')[0]; + const date = account.balanceDate + ? account.balanceDate.toISOString().split('T')[0] + : null; // Handle the null case appropriatelyTo verify whether
account.balanceDatecan benullorundefined, you can run the following script:This script searches the codebase for instances where
balanceDateis explicitly set tonullorundefined.
| function isKebabCase(str: string): boolean { | ||
| return /^[a-z]+(-[a-z]+)+$/.test(str); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving regex for edge cases.
The current implementation correctly identifies basic kebab-case strings. However, it doesn't handle some edge cases:
- It doesn't allow single-word lowercase strings (e.g., "word").
- It doesn't handle strings with numbers (e.g., "kebab-case2").
Consider updating the regex to handle these cases:
-function isKebabCase(str: string): boolean {
- return /^[a-z]+(-[a-z]+)+$/.test(str);
+function isKebabCase(str: string): boolean {
+ return /^[a-z0-9]+(-[a-z0-9]+)*$/.test(str);
}This updated regex allows single words and includes numbers, which might be more flexible for real-world usage.
📝 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.
| function isKebabCase(str: string): boolean { | |
| return /^[a-z]+(-[a-z]+)+$/.test(str); | |
| } | |
| function isKebabCase(str: string): boolean { | |
| return /^[a-z0-9]+(-[a-z0-9]+)*$/.test(str); | |
| } |
| @@ -5,10 +5,12 @@ | |||
| "roots": ["<rootDir>"], | |||
| "moduleFileExtensions": ["ts", "js", "json"], | |||
| "testEnvironment": "node", | |||
| "collectCoverage": true, | |||
| "collectCoverage": false, | |||
There was a problem hiding this comment.
💡 Codebase verification
Disabling Coverage Collection May Result in Missing Coverage Metrics
Setting collectCoverage to false in jest.config.json will prevent Jest from collecting coverage information during test runs. Since there are no alternative processes or CI configurations handling coverage, this change could lead to a lack of visibility into test coverage, potentially impacting code quality assessments.
🔗 Analysis chain
Consider the implications of disabling coverage collection.
Setting collectCoverage to false will speed up test execution, but it also means that coverage information won't be automatically collected. This could make it harder to track test coverage over time.
To ensure this change doesn't negatively impact your development process, please verify:
- Is there an alternative process in place to collect coverage information?
- Are there CI/CD pipelines that might rely on automatic coverage collection?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any scripts or CI configurations that might be affected by this change
grep -r "collectCoverage" .github/workflows package.json
Length of output: 213
Script:
#!/bin/bash
# Check if there are any scripts or CI configurations that might be affected by this change
grep -r "collectCoverage" .github/workflows package.json
Length of output: 56
Script:
#!/bin/bash
# Search for 'collectCoverage' in the entire codebase
grep -r "collectCoverage" .
Length of output: 140
| static fromJson(json: string): Organization { | ||
| const data = JSON.parse(json); | ||
| const camelCaseData = { | ||
| ...data, | ||
| sfinUrl: data['sfin-url'], | ||
| }; | ||
|
|
||
| return new Organization(camelCaseData); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and input validation
The fromJson method successfully converts a JSON string to an Organization instance. However, there are a few areas for improvement:
- Error handling: Add a try-catch block to handle potential JSON parsing errors.
- Input validation: Ensure that the required
sfinUrlfield is present in the input.
Here's a suggested implementation with these improvements:
static fromJson(json: string): Organization {
try {
const data = JSON.parse(json);
const camelCaseData = {
...data,
sfinUrl: data['sfin-url'],
};
if (!camelCaseData.sfinUrl) {
throw new Error("Missing required field 'sfin-url'");
}
return new Organization(camelCaseData);
} catch (error) {
throw new Error(`Failed to create Organization from JSON: ${error.message}`);
}
}This implementation will provide more robust error handling and ensure that the required sfinUrl field is present.
| static fromJson(json: string): Transaction { | ||
| const data = JSON.parse(json); | ||
| return new Transaction(data); | ||
| } |
There was a problem hiding this comment.
Add error handling to fromJson method for improved robustness.
While the fromJson method provides a convenient way to create a Transaction from JSON, it lacks error handling for invalid JSON or missing required fields.
Consider adding error handling to make the method more robust:
static fromJson(json: string): Transaction {
try {
const data = JSON.parse(json);
if (typeof data !== 'object' || data === null) {
throw new Error('Invalid JSON structure');
}
// You could add more specific checks here for required fields
return new Transaction(data);
} catch (error) {
if (error instanceof SyntaxError) {
throw new Error('Invalid JSON string');
}
throw error;
}
}This implementation will catch JSON parsing errors and also ensure that the parsed data is an object before attempting to create a Transaction instance.
| transports: [ | ||
| new winston.transports.Console({ silent: !config.useRequestLogger }), | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: useRequestLogger is not exported in src/load-config.js.
While useRequestLogger is defined within src/load-config.js, it is not exported as part of the config object. To ensure that middlewares.js can access and utilize this configuration, please update the export in src/load-config.js to include useRequestLogger.
- File:
src/load-config.js - Action: Add
useRequestLoggerto the exportedconfigobject.
🔗 Analysis chain
LGTM: Configurable logging implemented.
The addition of the silent option based on config.useRequestLogger is a good improvement, allowing for flexible logging control across different environments.
Please ensure that config.useRequestLogger is properly set in the load-config.js file. You can verify this with the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence and usage of useRequestLogger in load-config.js
# Test: Search for useRequestLogger in load-config.js
rg --type js 'useRequestLogger' ../load-config.js
# Test: Check if useRequestLogger is exported
ast-grep --lang javascript --pattern 'export const config = { $$$, useRequestLogger: $_, $$$}' ../load-config.js
Length of output: 287
Script:
#!/bin/bash
# Description: Locate load-config.js within the repository
# Search for load-config.js in the entire repository
fd load-config.js
# If found, verify the presence of useRequestLogger in the located file
# Replace <path_to_load-config.js> with the actual path from the previous command
# Example:
# ast-grep --lang javascript --pattern 'export const config = { $$$, useRequestLogger: $_, $$$}' <path_to_load-config.js>
Length of output: 38
Script:
#!/bin/bash
# Description: Verify the presence and usage of useRequestLogger in src/load-config.js
# Test: Search for useRequestLogger in src/load-config.js
rg --type js 'useRequestLogger' src/load-config.js
# Test: Check if useRequestLogger is exported within the config object
ast-grep --lang javascript --pattern 'export const config = { $$$, useRequestLogger: $_, $$$}' src/load-config.js
Length of output: 221
| async fetchAccessKey() { | ||
| const url = this.context.accessKeyUrl(); | ||
| const response = await this.http_client.request(url, { | ||
| method: this.context.method, | ||
| port: this.context.port, | ||
| headers: this.context.headers, | ||
| }); | ||
| this.context.accessKey = response; | ||
| } |
There was a problem hiding this comment.
Verify context and parse access key after fetching
In the fetchAccessKey method:
this.contextis used without checking if it's defined.- After fetching the access key,
parseAccessKeyis not called to extract necessary authentication details.
These omissions can lead to runtime errors and authentication issues. Please ensure this.context is set and parseAccessKey is called after obtaining the access key.
Apply this diff to address these concerns:
+ if (!this.context) {
+ throw new GenericSimplefinError('Context must be set before fetching the access key.');
+ }
const url = this.context.accessKeyUrl();
let response;
try {
response = await this.http_client.request(url, {
method: this.context.method,
port: this.context.port,
headers: this.context.headers,
});
} catch (error) {
throw new GenericSimplefinError(`Failed to fetch access key: ${error.message}`);
}
this.context.accessKey = response;
+ this.context.parseAccessKey(this.context.accessKey);📝 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.
| async fetchAccessKey() { | |
| const url = this.context.accessKeyUrl(); | |
| const response = await this.http_client.request(url, { | |
| method: this.context.method, | |
| port: this.context.port, | |
| headers: this.context.headers, | |
| }); | |
| this.context.accessKey = response; | |
| } | |
| async fetchAccessKey() { | |
| if (!this.context) { | |
| throw new GenericSimplefinError('Context must be set before fetching the access key.'); | |
| } | |
| const url = this.context.accessKeyUrl(); | |
| let response; | |
| try { | |
| response = await this.http_client.request(url, { | |
| method: this.context.method, | |
| port: this.context.port, | |
| headers: this.context.headers, | |
| }); | |
| } catch (error) { | |
| throw new GenericSimplefinError(`Failed to fetch access key: ${error.message}`); | |
| } | |
| this.context.accessKey = response; | |
| this.context.parseAccessKey(this.context.accessKey); | |
| } |
| parseAccessKey(accessKey: string) { | ||
| let scheme = null; | ||
| let rest = null; | ||
| let auth = null; | ||
| let username = null; | ||
| let password = null; | ||
| let baseUrl = null; | ||
| [scheme, rest] = accessKey.split('//'); | ||
| [auth, rest] = rest.split('@'); | ||
| [username, password] = auth.split(':'); | ||
| baseUrl = `${scheme}//${rest}`; | ||
|
|
||
| this.username = username; | ||
| this.password = password; | ||
| this.baseUrl = baseUrl; | ||
| } |
There was a problem hiding this comment.
Add validation in parseAccessKey to handle malformed input
The parseAccessKey method assumes the accessKey string has a specific format. If the accessKey doesn't match the expected format, the method could throw errors or produce incorrect values. Please add validation checks and handle potential exceptions to make the method more robust.
Consider updating the method as follows:
parseAccessKey(accessKey: string) {
+ if (!accessKey.includes('//') || !accessKey.includes('@') || !accessKey.includes(':')) {
+ throw new GenericSimplefinError('Invalid access key format.');
+ }
let scheme = null;
let rest = null;
let auth = null;
let username = null;
let password = null;
let baseUrl = null;
[scheme, rest] = accessKey.split('//');
[auth, rest] = rest.split('@');
[username, password] = auth.split(':');
baseUrl = `${scheme}//${rest}`;
this.username = username;
this.password = password;
this.baseUrl = baseUrl;
}📝 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.
| parseAccessKey(accessKey: string) { | |
| let scheme = null; | |
| let rest = null; | |
| let auth = null; | |
| let username = null; | |
| let password = null; | |
| let baseUrl = null; | |
| [scheme, rest] = accessKey.split('//'); | |
| [auth, rest] = rest.split('@'); | |
| [username, password] = auth.split(':'); | |
| baseUrl = `${scheme}//${rest}`; | |
| this.username = username; | |
| this.password = password; | |
| this.baseUrl = baseUrl; | |
| } | |
| parseAccessKey(accessKey: string) { | |
| if (!accessKey.includes('//') || !accessKey.includes('@') || !accessKey.includes(':')) { | |
| throw new GenericSimplefinError('Invalid access key format.'); | |
| } | |
| let scheme = null; | |
| let rest = null; | |
| let auth = null; | |
| let username = null; | |
| let password = null; | |
| let baseUrl = null; | |
| [scheme, rest] = accessKey.split('//'); | |
| [auth, rest] = rest.split('@'); | |
| [username, password] = auth.split(':'); | |
| baseUrl = `${scheme}//${rest}`; | |
| this.username = username; | |
| this.password = password; | |
| this.baseUrl = baseUrl; | |
| } |
| import { SimpleFinService } from './services/simplefin-service.ts'; | ||
| import { SimplefinApi } from './services/simplefin-api.ts'; | ||
| import { HttpsClient } from './httpClient.ts'; |
There was a problem hiding this comment.
Ensure Correct Module Resolution by Removing '.ts' Extensions
The import statements on lines 6-8 include the '.ts' extension while importing modules into a JavaScript file. This can cause issues during module resolution in Node.js since the runtime typically resolves modules without extensions or with '.js' extensions.
Apply the following diff to remove the '.ts' extensions:
-import { SimpleFinService } from './services/simplefin-service.ts';
-import { SimplefinApi } from './services/simplefin-api.ts';
-import { HttpsClient } from './httpClient.ts';
+import { SimpleFinService } from './services/simplefin-service';
+import { SimplefinApi } from './services/simplefin-api';
+import { HttpsClient } from './httpClient';Alternatively, if this file is intended to be a TypeScript file, consider renaming app-simplefin.js to app-simplefin.ts for consistency.
📝 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.
| import { SimpleFinService } from './services/simplefin-service.ts'; | |
| import { SimplefinApi } from './services/simplefin-api.ts'; | |
| import { HttpsClient } from './httpClient.ts'; | |
| import { SimpleFinService } from './services/simplefin-service'; | |
| import { SimplefinApi } from './services/simplefin-api'; | |
| import { HttpsClient } from './httpClient'; |
| secretsService.set(SecretName.simplefin_accessKey, accessKey); | ||
| if (accessKey == null || accessKey === 'Forbidden') { | ||
| throw new Error('No access key'); | ||
| } | ||
| } | ||
| } | ||
| //eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Disabling ESLint Rules by Handling Unused Error Variable
The directive //eslint-disable-next-line @typescript-eslint/no-unused-vars suppresses the linter warning for the unused error variable in the catch block. Instead of disabling the ESLint rule, consider prefixing the variable name with an underscore to indicate that it is intentionally unused.
Apply this diff to rename the variable:
- } catch (error) {
+ } catch (_error) {Alternatively, if it's valuable to log the error for debugging purposes, you can use the error variable:
} catch (error) {
+ console.error(error);Committable suggestion was skipped due to low confidence.
| @@ -104,7 +104,7 @@ describe('/reset-user-file', () => { | |||
| fileId, | |||
| ]); | |||
|
|
|||
| expect(rows[0].group_id).toBeNull; | |||
| expect(rows[0].group_id).toBeNull(); | |||
There was a problem hiding this comment.
Fix the assertion to properly verify the file deletion status
The test intends to verify that the file is marked as deleted, but it's currently checking if group_id is null. You should check the deleted field to confirm that the file is marked as deleted.
Apply this diff to correct the assertion:
- expect(rows[0].group_id).toBeNull();
+ expect(rows[0].deleted).toBe(1);📝 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.
| expect(rows[0].group_id).toBeNull(); | |
| expect(rows[0].deleted).toBe(1); |
Changes
splits app-simplefin.js into multiple components:
HTTPSClientwhich is responsible for calling any external URL, using PromisesSimplefinAPIwhich uses the injectedhttp_clientto call the simplefin API, and parse its response into one of the objects inmodels/SimplefinServicewhich uses the injectedSimplefinAPIto do all the business logic for Actual. Currently, I haven't moved any of the business logic fromapp-simplefin.js, because I haven't tested these, so I did not want to break stuff. However, the end goal is to have everything except the routing in this service, to that we can test inner logic of the service, with calling our API, nor Simplefin'sSimplefinAPIimplementation withFakeSimplefinAPIwhen integration testing theSimplefinService. This will help us in the long run, so that we don't run into mocking hell where everything is mocked and we're not testing the actual code, but the mocks.cleans up the standard testing output:
--silentflag injest.config.jsonso that the test output is not littered will all the transactions logging fromapp-gocardlessNODE_NO_WARNINGS=1to disable the ExperimentalWarnings from VM modules coming from the--experimental-vm-modulesflag. Currently, in Node 18 and 20, there is no way to disable this specific warning (or other warning as a matter of fact) directly from the node. [Source]. Instead, some people create jest setup scripts that mockconsole.warnand filter these warnings. I tried it, it didn't work, so I resorted to the nuclear option that disables all warnings. This is not ideal, but I'm not knowledgeable enough in the JS ecosystem to make it work.Now, a standard
npm run testoutput looks all green and clean:allows us to run typescript files directly by calling
npx tsx app.js.node app.jsdoes not allow importing.tsfiles likesimplefin-service.ts. I noticed that we were not running the files from thebuild/folder either way (as we were runningnode app.js), so JavaScript code built from the Typescript code was not used. Might as well run typescript directly.This required changing the Dockerfiles too.
jest(which usests-nodeunder the hood).I admit, this was a mess to deal with, lots of flags changed in
jest.config.jsonandtsconfig.json. I would have preferred changing only the code, but I desperately wanted to introduce typescript in the repo, and now it's done. I'm sure this can be improved.I will need someone to manually test the simplefin integration, to make sure that the current refactor hasn't broken anything. I'm not able to do it, as I don't have a Simplefin Account.
Future
app-simplefin.jstoapp-simplefin/services/simplefin-service.js. I prefer waiting until @psybers merges his PR Sync multiple accounts in a single SimpleFIN API call. #384 so that I don't break any existing/upcoming features.