Alquimia #21
Closed
Alquimia #21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Instantiate objects globally only when necessary, Move all http requests into httpService, Delete console.logs
Transcribir PlayersController.create
Validate index params, Move authorization logic into DAO
Fixes/http n token services
Add bank accounts CRUD
Player fix
Add web push support,
Require authentication on /auth/logout
Add Roles table, Move role out of JWT payload and into user object
* Fix double refresh hanging the server * 🧹 Clean up * Add console.error() to exitLog
* Add alquimia payment verification * Update seed.sql * Comment out services.old.ts * Remove Player from DepositDAO.create() * Remove Player from DepositDAO.create()
WalkthroughThe project underwent significant enhancements, focusing on database integration, encryption, API integrations, and extensive testing. Changes include the addition of MySQL connectivity, encryption configurations, basic authentication, API tokens, and updates to the PM2 ecosystem. The renaming of the project to "Timba Api" brought new functionalities and documentation updates. Test coverage expanded across various functionalities, and the database schema was extensively modified to support new features. The source code saw comprehensive updates to accommodate these changes. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 42
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
docker-compose.ymlis excluded by:!**/*.ymlpackage.jsonis excluded by:!**/*.jsonprisma/migrations/migration_lock.tomlis excluded by:!**/*.tomlyarn.lockis excluded by:!**/*.lock
Files selected for processing (103)
- .env.example (2 hunks)
- .gitignore (2 hunks)
- .vscode/settings.json (1 hunks)
- README.md (2 hunks)
- tests/agent.test.ts (1 hunks)
- tests/alq-token.service.test.ts (1 hunks)
- tests/auth.test.ts (1 hunks)
- tests/bank-account.test.ts (1 hunks)
- tests/finance.service.test.ts (1 hunks)
- tests/home.test.ts (2 hunks)
- tests/http.service.test.ts (1 hunks)
- tests/player.test.ts (1 hunks)
- tests/transactions.test.ts (1 hunks)
- tests/web-push.test.ts (1 hunks)
- ecosystem.config.js (2 hunks)
- prisma/migrations/20240220155527_initial_migration/migration.sql (1 hunks)
- prisma/migrations/20240222132000_drop_bank_accounts/migration.sql (1 hunks)
- prisma/migrations/20240222133838_drop_bank_account_indexes/migration.sql (1 hunks)
- prisma/migrations/20240222133915_add_bank_accounts/migration.sql (1 hunks)
- prisma/migrations/20240222140003_add_player_and_owner_to_bank_account/migration.sql (1 hunks)
- prisma/migrations/20240222140931_make_bank_alias_nullable/migration.sql (1 hunks)
- prisma/migrations/20240222203552_replace_panel_id_foreign_key_with_id/migration.sql (1 hunks)
- prisma/migrations/20240223113516_make_user_root_id_int/migration.sql (1 hunks)
- prisma/migrations/20240223184220_add_currency_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240223185921_add_currency_to_payments/migration.sql (1 hunks)
- prisma/migrations/20240226180926_add_dirty_flag_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240227170435_add_bank_account_to_user_root/migration.sql (1 hunks)
- prisma/migrations/20240228123536_add_tokens_table/migration.sql (1 hunks)
- prisma/migrations/20240304132411_add_coins_transfered_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240304210104_create_web_push_subscriptions_table/migration.sql (1 hunks)
- prisma/migrations/20240304214515_stretch_endpoint_and_add_exp_time_to_push_subscriptions/migration.sql (1 hunks)
- prisma/migrations/20240305132627_add_timestamps_to_token_and_pushsub/migration.sql (1 hunks)
- prisma/migrations/20240306190737_add_user_agent_to_token/migration.sql (1 hunks)
- prisma/migrations/20240307110322_make_user_agent_string_nullable/migration.sql (1 hunks)
- prisma/migrations/20240311143228_create_roles_table/migration.sql (1 hunks)
- prisma/migrations/20240311143619_change_role_name_data_type/migration.sql (1 hunks)
- prisma/migrations/20240311143934_make_role_name_unique/migration.sql (1 hunks)
- prisma/migrations/20240314202610_create_bot_messages_table/migration.sql (1 hunks)
- prisma/migrations/20240314204706_add_menus_to_bot_messages_table/migration.sql (1 hunks)
- prisma/migrations/20240322121803_add_alq_tokens_to_user_root/migration.sql (1 hunks)
- prisma/migrations/20240322140747_enlarge_alq_api_manager_col_size/migration.sql (1 hunks)
- prisma/migrations/20240322201756_re_build_deposits_table/migration.sql (1 hunks)
- prisma/migrations/20240322202436_add_status_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240322202948_add_tracking_number_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240322213651_remove_confirmed_at_and_delted_at_from_deposits/migration.sql (1 hunks)
- prisma/migrations/20240322222531_add_amount_to_deposits/migration.sql (1 hunks)
- prisma/migrations/20240323124705_make_deposit_tracking_number_unique/migration.sql (1 hunks)
- prisma/schema.prisma (1 hunks)
- seed.sql (1 hunks)
- src/app.ts (2 hunks)
- src/chatbot/flows.ts (1 hunks)
- src/chatbot/messages.ts (1 hunks)
- src/components/agent/controller.ts (1 hunks)
- src/components/agent/index.ts (1 hunks)
- src/components/agent/services.ts (1 hunks)
- src/components/agent/validators.ts (1 hunks)
- src/components/auth/controller.ts (1 hunks)
- src/components/auth/services.ts (1 hunks)
- src/components/auth/validators.ts (1 hunks)
- src/components/bank-accounts/controller.ts (1 hunks)
- src/components/bank-accounts/index.ts (1 hunks)
- src/components/bank-accounts/services.ts (1 hunks)
- src/components/bank-accounts/validators.ts (1 hunks)
- src/components/players/controller.ts (1 hunks)
- src/components/players/index.ts (1 hunks)
- src/components/players/services.ts (1 hunks)
- src/components/players/validators.ts (1 hunks)
- src/components/transactions/controller.ts (1 hunks)
- src/components/transactions/index.ts (1 hunks)
- src/components/transactions/services.old.ts (1 hunks)
- src/components/transactions/services.ts (1 hunks)
- src/components/transactions/validators.ts (1 hunks)
- src/components/web-push/controller.ts (1 hunks)
- src/components/web-push/services.ts (1 hunks)
- src/components/web-push/validators.ts (1 hunks)
- src/config/errors.ts (1 hunks)
- src/config/index.ts (3 hunks)
- src/db/agent.ts (1 hunks)
- src/db/bank-accounts.ts (1 hunks)
- src/db/deposits.ts (1 hunks)
- src/db/payments.ts (1 hunks)
- src/db/players.ts (1 hunks)
- src/db/seed.ts (1 hunks)
- src/db/token.ts (1 hunks)
- src/db/transactions.ts (1 hunks)
- src/db/user-root.ts (1 hunks)
- src/db/web-push.ts (1 hunks)
- src/helpers/apiResponse.ts (1 hunks)
- src/helpers/loggers.ts (4 hunks)
- src/helpers/notification.ts (1 hunks)
- src/index.ts (1 hunks)
- src/middlewares/auth.ts (1 hunks)
- src/middlewares/errorHandler.ts (3 hunks)
- src/middlewares/requestErrorHandler.ts (1 hunks)
- src/routes/agent.router.ts (1 hunks)
- src/routes/auth.router.ts (1 hunks)
- src/routes/bank-accounts.router.ts (1 hunks)
- src/routes/home.router.ts (1 hunks)
- src/routes/index.ts (1 hunks)
- src/routes/players.router.ts (1 hunks)
- src/routes/transactions.router.ts (1 hunks)
- src/routes/web-push.ts (1 hunks)
- src/server.ts (2 hunks)
Files not processed due to max files limit (27)
- src/services/alquimia-token.service.ts
- src/services/casino-token.service.ts
- src/services/external-api.service.ts
- src/services/http.service.ts
- src/services/jwt.service.ts
- src/types/bot/bot-whatsapp-database-baileys.d.ts
- src/types/bot/bot-whatsapp-provider-mock.d.ts
- src/types/bot/bot-whatsapp.d.ts
- src/types/global.d.ts
- src/types/request/bank-account.ts
- src/types/request/players.ts
- src/types/request/token.ts
- src/types/request/transfers.ts
- src/types/request/user-root.ts
- src/types/request/web-push.ts
- src/types/response/agent.ts
- src/types/response/alquimia.ts
- src/types/response/bankAccount.ts
- src/types/response/error.ts
- src/types/response/jwt.ts
- src/types/response/players.ts
- src/types/response/transactions.ts
- src/types/response/transfers.ts
- src/types/services/http.ts
- src/utils/auth.ts
- src/utils/crypt.ts
- src/utils/parser.ts
Files skipped from review due to trivial changes (4)
- src/components/agent/index.ts
- src/components/bank-accounts/index.ts
- src/components/players/index.ts
- src/components/transactions/index.ts
Additional comments: 137
src/db/agent.ts (1)
- 1-1: The
AgentDAOclass is currently empty. If this class is intended as a placeholder for future development, consider adding a comment or documentation to clarify its purpose. If it's not needed, it might be best to remove it to keep the codebase clean.prisma/migrations/20240322222531_add_amount_to_deposits/migration.sql (1)
- 2-2: The migration script adds a
DOUBLEtype column foramount. For financial amounts, consider using a more precise data type likeDECIMALto avoid potential precision issues. Also, ensure that the handling of null values aligns with the application's logic and requirements.prisma/migrations/20240311143619_change_role_name_data_type/migration.sql (1)
- 2-2: The migration script correctly modifies the
namecolumn in theROLEStable toVARCHAR(191)and ensures it is not nullable. Verify the impact on existing data and ensure the chosen length aligns with the application's requirements and database indexing optimizations.prisma/migrations/20240307110322_make_user_agent_string_nullable/migration.sql (1)
- 2-2: The migration script correctly modifies the
user_agentcolumn in theTOKENStable to allow null values. Ensure that the application's logic and token validation processes are updated to handle null values appropriately.prisma/migrations/20240227170435_add_bank_account_to_user_root/migration.sql (1)
- 2-2: The migration script adds a
JSONtype column forbankAccount. While JSON allows for flexible data storage, consider the implications for data integrity and querying efficiency. Ensure that the handling of null values aligns with the application's logic and requirements.prisma/migrations/20240322140747_enlarge_alq_api_manager_col_size/migration.sql (1)
- 2-2: The migration script correctly modifies the
alq_api_managercolumn in theUSERS_ROOTtable to aTEXTdata type. Consider the security implications of storing potentially sensitive information in this column and ensure that the handling of null values aligns with the application's logic and requirements.prisma/migrations/20240304132411_add_coins_transfered_to_deposits/migration.sql (1)
- 2-2: The migration script correctly adds a
DATETIME(3)type column forcoins_transfered. Ensure that the application's logic is updated to handle null values appropriately and that the precision level ofDATETIME(3)aligns with the application's requirements.prisma/migrations/20240226180926_add_dirty_flag_to_deposits/migration.sql (1)
- 2-2: The migration script correctly adds a
BOOLEANtype columndirtywith a default value oftrue. Ensure that the application's logic is updated to handle this new flag appropriately, both for existing records and new insertions.prisma/migrations/20240223184220_add_currency_to_deposits/migration.sql (1)
- 1-2: The SQL statement to add a
currencycolumn to theDEPOSITStable is syntactically correct. However, be aware that adding a NOT NULL column with a default value to a potentially large existing table could lock the table and impact performance during the operation. Consider applying this change during a maintenance window or when database usage is low to minimize impact.prisma/migrations/20240223185921_add_currency_to_payments/migration.sql (1)
- 1-2: The SQL statement to add a
currencycolumn to thePAYMENTStable is correct. As with any alteration to large existing tables, consider the potential performance implications and plan the migration accordingly to minimize impact.prisma/migrations/20240322121803_add_alq_tokens_to_user_root/migration.sql (1)
- 1-3: The SQL statement to add
alq_api_managerandalq_tokencolumns to theUSERS_ROOTtable is correct. Ensure that the size of VARCHAR(800) is based on the expected length of the data to be stored, especially for tokens, which might be encrypted or encoded. Adjusting column sizes to match the data can improve storage efficiency and performance.prisma/migrations/20240304214515_stretch_endpoint_and_add_exp_time_to_push_subscriptions/migration.sql (1)
- 1-3: The SQL statement to modify the
endpointcolumn and add anexpirationTimecolumn to theWEB_PUSH_SUBSCRIPTIONStable is correct. IfexpirationTimeis intended to store timestamps, consider using the TIMESTAMP data type for more explicit intent and potential benefits in date-time operations.prisma/migrations/20240314202610_create_bot_messages_table/migration.sql (1)
- 1-7: The SQL statement to create the
BOT_MESSAGEStable withidandmessagescolumns is correct. When using the JSON data type formessages, consider the potential performance implications and ensure that your application logic properly handles the structured data. Additionally, ensure that theidcolumn size aligns with the expected format of the identifiers.src/components/auth/validators.ts (1)
- 1-11: The validation schema for the token in
validateTokenfunction is well-implemented, ensuring that the token is a non-empty string and is trimmed. This is a good practice for input validation, enhancing the security and robustness of the authentication process.prisma/migrations/20240311143934_make_role_name_unique/migration.sql (1)
- 8-8: Ensure that the
ROLEStable does not contain any duplicatenamevalues before applying this migration to prevent failure.prisma/migrations/20240306190737_add_user_agent_to_token/migration.sql (1)
- 8-8: Before applying this migration, ensure the
TOKENStable is either empty or consider providing a default value for existing rows to avoid issues.prisma/migrations/20240322202948_add_tracking_number_to_deposits/migration.sql (1)
- 8-8: Before applying this migration, ensure the
DEPOSITStable is either empty or consider providing a default value for existing rows to avoid issues.prisma/migrations/20240323124705_make_deposit_tracking_number_unique/migration.sql (1)
- 8-8: Ensure that the
DEPOSITStable does not contain any duplicatetracking_numbervalues before applying this migration to prevent failure.src/middlewares/requestErrorHandler.ts (1)
- 4-7: Ensure proper typing for
Req,Res, andNextFnparameters to enhance type safety and code readability.Consider importing types from Express, such as
Request,Response, andNextFunction.prisma/migrations/20240304210104_create_web_push_subscriptions_table/migration.sql (1)
- 1-9: The creation of the
WEB_PUSH_SUBSCRIPTIONStable with a unique index onendpointand a JSON column forkeyslooks good. Ensure that the JSON structure forkeysis well-documented and validated upon insertion to maintain data integrity.prisma/migrations/20240322213651_remove_confirmed_at_and_delted_at_from_deposits/migration.sql (1)
- 9-10: Dropping the
confirmed_atanddeleted_atcolumns will permanently remove these fields and their data. Ensure that this action aligns with the business requirements and consider archiving or migrating the data if necessary.prisma/migrations/20240222140931_make_bank_alias_nullable/migration.sql (3)
- 2-2: Ensure that dropping the foreign key
BANK_ACCOUNTS_owner_id_fkeydoes not impact any existing functionalities or data integrity. It's crucial to verify that this change aligns with the overall database schema evolution strategy.- 5-5: Making
bankAliasnullable is a significant change. Confirm that all application logic interacting with theBANK_ACCOUNTStable correctly handlesNULLvalues forbankAliasto prevent unexpected behavior.- 8-8: Adding a new foreign key constraint
BANK_ACCOUNTS_player_id_fkeyintroduces a tighter coupling betweenBANK_ACCOUNTSandPLAYERS. Ensure that this change is thoroughly tested, especially regardingON DELETE RESTRICTandON UPDATE CASCADEbehaviors, to avoid unintended data loss or inconsistency.src/index.ts (1)
- 8-10: Starting the WhatsApp bot only in the production environment is a significant operational change. Ensure that this behavior is documented and that any necessary configurations are in place for the production environment. Additionally, consider the implications for staging or other non-production environments where WhatsApp bot functionality might be required for testing.
prisma/migrations/20240228123536_add_tokens_table/migration.sql (2)
- 2-9: The creation of the
TOKENStable with fields forid,invalid,next, andplayer_idseems well-structured. However, ensure that theidfield's length and the choice ofVARCHAR(191)are consistent with the application's requirements for token identification. Also, confirm that thenextfield's purpose and usage are clearly documented, as its name alone does not convey its function.- 12-12: Adding a foreign key constraint from
TOKENStoPLAYERSis crucial for maintaining referential integrity. Ensure that theON DELETE RESTRICTandON UPDATE CASCADEoptions are aligned with the application's data management strategy, particularly in scenarios where a player's record might be updated or deleted.src/helpers/apiResponse.ts (1)
- 4-13: Introducing an
errorMessageparameter and conditional logic for handling falsydatawith a providederrorMessageis a useful enhancement for error handling. However, ensure that this change is consistently applied across all API endpoints to maintain uniformity in error responses. Additionally, consider using a more specific HTTP status code thanNOT_FOUNDfor cases wheredatais falsy but not necessarily indicative of a resource not being found, such as validation errors or other business logic conditions.prisma/migrations/20240223113516_make_user_root_id_int/migration.sql (1)
- 9-11: Changing the
idcolumn type fromVarChar(191)toINTEGERand setting it asAUTO_INCREMENTin theUSERS_ROOTtable is a significant structural change. Ensure that this alteration is thoroughly tested, especially regarding data migration and the impact on existing records. Additionally, confirm that all application logic interacting with theUSERS_ROOTtable is updated to accommodate the newidcolumn type.prisma/migrations/20240222132000_drop_bank_accounts/migration.sql (1)
- 8-17: Dropping the
BANK_ACCOUNTStable is a major change that will result in the loss of all data within it. Ensure that this action is part of a well-considered data migration strategy, and that backups are made before proceeding. Additionally, verify that all application logic that previously interacted with theBANK_ACCOUNTStable is either removed or updated to reflect this change.src/middlewares/auth.ts (1)
- 4-14: Introducing middleware functions for role-based access control (
requireAgentRoleandrequireUserRole) enhances the application's security posture. However, ensure that the use ofreq.user!.rolesis safe and that there's a preceding middleware to guaranteereq.useris defined and contains arolesproperty. Additionally, consider centralizing role names in a single source of truth to avoid hard-coded strings throughout the codebase, which can reduce maintainability and increase the risk of typos.prisma/migrations/20240222133838_drop_bank_account_indexes/migration.sql (1)
- 9-18: Dropping indexes and the
bank_accountcolumn from theDEPOSITSandPAYMENTStables is a significant change that affects the database schema and potentially the application's functionality. Ensure that this change is part of a broader schema evolution strategy and that all application logic previously relying on these indexes and columns is updated accordingly. Additionally, verify that the removal of these elements does not adversely affect the application's performance or data integrity.prisma/migrations/20240222140003_add_player_and_owner_to_bank_account/migration.sql (1)
- 16-17: Adding required columns
ownerandplayer_idwithout default values to theBANK_ACCOUNTStable is highlighted in the warning. Ensure that the table is empty (as ensured by the deletion operation) or consider providing a default value or a migration strategy for existing rows to prevent issues.src/routes/index.ts (1)
- 1-20: The setup of the main router and the import of individual routers are correctly implemented. This structure promotes modularity and makes the routing easier to manage and understand.
src/routes/auth.router.ts (1)
- 10-26: The setup of authentication routes, including the use of
passportfor JWT authentication and custom middleware for validation and error handling, is correctly implemented. This approach ensures secure and robust authentication handling.__tests__/finance.service.test.ts (1)
- 1-26: The test setup and the logic for verifying a payment in the
FinanceServicesclass are correctly implemented. This test ensures that theverifyPaymentmethod functions as expected by checking the truthiness of theamountproperty in the response.src/components/bank-accounts/services.ts (1)
- 1-28: The
BankAccountServicesclass correctly implements CRUD operations for bank accounts, using the DAO pattern for database interactions. This approach promotes separation of concerns and makes the codebase more maintainable.prisma/migrations/20240222203552_replace_panel_id_foreign_key_with_id/migration.sql (1)
- 11-11: Ensure that dropping the
BANK_ACCOUNTS_owner_id_fkeyindex does not adversely affect query performance on theBANK_ACCOUNTStable..vscode/settings.json (1)
- 2-22: The VS Code settings customization enhances the developer experience. Consider documenting these settings in the project's README or developer guide to ensure new team members are aware of the recommended setup.
src/components/auth/controller.ts (1)
- 9-10: Consider validating or sanitizing the
tokenanduser_idinputs to mitigate potential security risks.Also applies to: 24-24
src/routes/players.router.ts (1)
- 14-14: The comment "Post para crear usuarios" might be misleading or incomplete. Consider refining it for clarity and ensuring consistency with the project's primary language.
src/routes/web-push.ts (1)
- 17-17: LGTM! The use of
requireAgentRolemiddleware for role enforcement on web push subscription endpoints is a good security practice.__tests__/alq-token.service.test.ts (1)
- 10-34: The test suite covers essential functionalities. Consider adding negative test cases and tests for error handling scenarios to improve coverage and robustness.
ecosystem.config.js (1)
- 4-4: The updates to the app name and dynamic port setting in
ecosystem.config.jsare well-implemented and align with best practices for deployment flexibility.Also applies to: 22-22
src/components/agent/validators.ts (1)
- 28-50: Using a reusable configuration for optional string fields is a good practice for maintainability and reducing redundancy.
__tests__/http.service.test.ts (2)
- 14-32: The test suite for
authedAlqApiis well-structured and correctly tests the expected functionality.- 35-41: The test suite for
authedAgentApiis correctly implemented and tests the expected functionality.src/components/web-push/controller.ts (3)
- 10-19: The
indexmethod is correctly implemented, with appropriate use of HTTP status codes and error handling.- 21-31: The
createmethod is correctly implemented, with appropriate use of theCREATEDHTTP status code and error handling.- 33-43: The
deletemethod is correctly implemented, with appropriate use of theOKHTTP status code and error handling.src/app.ts (1)
- 3-23: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-47]
The configuration and setup of the Express application are correctly implemented, with appropriate security measures, middleware usage, and dynamic CORS setup based on the environment. The passport setup and API route prefixing are also correctly handled.
.env.example (1)
- 15-32: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-50]
The environment variable configuration in
.env.exampleis well-structured and follows best practices for security and clarity, with appropriate placeholders for sensitive information.src/routes/transactions.router.ts (5)
- 15-17: Authentication middleware is correctly applied to all routes in this router, ensuring that only authenticated users can access these endpoints.
- 18-18: The
requireUserRolemiddleware is applied globally to all routes in this router. Ensure that this middleware correctly handles different user roles and grants access as intended. It's important to verify that the role checking logic aligns with the requirements for each route.- 20-24: The deposit route correctly uses validators and middleware to handle requests. However, the
checkExact()middleware is used without any apparent configuration or purpose. Ensure that this middleware is necessary and correctly configured to avoid unintended behavior.Verification successful
Given the widespread use of
checkExactacross various routes without specific configuration, as indicated by the grep output, it's reasonable to infer that this middleware is intended to be used in a standard manner, likely for a common purpose related to request validation or sanitization. The consistent pattern of usage across different routes suggests thatcheckExactis correctly utilized in the deposit route as well, aligning with its application in other parts of the application. Therefore, the review comment questioning the necessity and configuration ofcheckExactmight be based on a misunderstanding of its role or a lack of context about its typical usage pattern.* 30-34: The delete deposit route is well-structured, using a validator to ensure the deposit ID is valid and middleware to handle errors. This approach promotes clean error handling and validation practices. * 36-41: Similar to the deposit route, the cashout route uses validators and middleware effectively. However, the same concern with `checkExact()` applies here. Verify its necessity and configuration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the implementation details of checkExact to ensure it's correctly utilized. grep -R "checkExact" src/Length of output: 1172
src/routes/bank-accounts.router.ts (5)
- 15-17: Authentication middleware is correctly applied to all routes in this router, ensuring that only authenticated users can access these endpoints.
- 18-18: The
requireUserRolemiddleware is applied globally to all routes in this router. Ensure that this middleware correctly handles different user roles and grants access as intended. It's important to verify that the role checking logic aligns with the requirements for each route.- 19-24: The index route for bank accounts uses validators and middleware effectively. However, the optional parameter
/:id?suggests that this route can fetch a single bank account or all bank accounts based on the presence of an ID. Ensure that the controller logic correctly handles both scenarios.Verification successful
To accurately verify how the
BankAccountsController.indexmethod handles both single and multiple bank account retrievals, we need more information about the implementation of theindexmethod withinbankAccountServicesand howplayer_idis managed. Let's proceed to gather this information.
The verification process confirms that the
BankAccountsController.indexmethod correctly handles both scenarios: fetching a single bank account when an ID is provided and fetching all bank accounts when no ID is present. This is achieved by conditionally calling eitherbankAccountServices.showfor a single account retrieval orbankAccountServices.indexfor retrieving all accounts associated with theplayer_id. Therefore, the controller logic is implemented as expected, addressing the concern raised in the initial review comment.* 33-38: The update route for bank accounts is well-structured, using a validator to ensure the request body is valid and middleware to handle errors. This approach promotes clean error handling and validation practices. * 40-45: The delete route for bank accounts correctly uses a validator for the bank account ID and middleware for error handling. This ensures that only valid requests proceed to the controller logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the implementation of BankAccountsController.index to ensure it handles both single and multiple bank account retrievals. grep -A 10 "index(" src/components/bank-accounts/controller.tsLength of output: 315
Script:
#!/bin/bash # Inspect the implementation of the index method within bankAccountServices grep -A 20 "index(" src/components/bank-accounts/services.ts # Additionally, check how player_id is determined in the BankAccountsController.index method grep -B 5 -A 5 "player_id" src/components/bank-accounts/controller.tsLength of output: 1827
src/server.ts (3)
- 3-5: The imports for
BotWhatsapp,MockAdapter, andProviderWSare correctly structured. Ensure that these components are properly implemented and secure, especially considering their involvement in bot functionality.- 30-39: The
startWhatsappBotfunction is well-structured, initializing the WhatsApp bot with a mock database adapter, a provider, and a flow. Ensure that the use ofMockAdapteris intended for production environments or if it should be conditional based on the environment (development vs. production).- 25-25: The server start message has been updated to reflect the project's new name, "Timba Api". This change is consistent with the PR objectives.
src/routes/agent.router.ts (5)
- 15-20: The login route for agents is correctly structured, using a validator for credentials and middleware for error handling. This ensures that only valid login requests are processed.
- 22-24: Authentication middleware is correctly applied to all subsequent routes after the login route, ensuring that only authenticated agents can access these endpoints.
- 25-25: The
requireAgentRolemiddleware is applied globally to all routes after authentication. Ensure that this middleware correctly identifies agent roles and grants access as intended. It's important to verify that the role checking logic aligns with the requirements for each route.- 27-32: The route for marking payments as paid uses a validator to ensure the payment ID is valid and middleware for error handling. This approach promotes clean error handling and validation practices.
- 37-42: The update bank account route is well-structured, using a validator to ensure the request body is valid and middleware to handle errors. This approach promotes clean error handling and validation practices.
src/components/transactions/validators.ts (3)
- 3-24: The
validateCashoutRequestvalidator correctly defines validation rules for cashout requests, including checks for amount, currency, and bank account. This ensures that only well-formed requests are processed.- 26-52: The
validateDepositRequestvalidator is well-structured, defining validation rules for deposit requests, including optional ID validation, currency, tracking number, and paid_at date. This comprehensive validation promotes secure and correct request processing.- 54-61: The
validateDepositIdvalidator correctly ensures that the deposit ID is an integer and not empty. This validation is crucial for routes that operate on specific deposits.prisma/migrations/20240222133915_add_bank_accounts/migration.sql (2)
- 1-7: The warnings at the beginning of the migration script highlight potential issues with adding non-nullable columns to possibly non-empty tables. Ensure that existing records in the
DEPOSITSandPAYMENTStables can accommodate this change, or consider a strategy to populate these columns for existing records.- 9-36: The migration script correctly adds the
bank_accountcolumn to theDEPOSITSandPAYMENTStables, creates theBANK_ACCOUNTStable with appropriate fields and constraints, and establishes foreign key relationships. Ensure that theupdated_atcolumn in theBANK_ACCOUNTStable is either auto-updated or managed correctly in application logic.src/components/web-push/services.ts (3)
- 21-26: Consider validating the presence of
CONFIG.AUTH.WEB_PUSH_PUBLIC_KEYat application startup rather than at runtime in thegetPublicKeymethod. This approach ensures that configuration issues are caught early, improving the application's reliability.- 31-40: The
createmethod directly passes thesubscriptionobject to theWebPushDAO.upsertmethod. Ensure that thesubscriptionobject is validated against expected schema to prevent potential injection attacks or data integrity issues.Verification successful
The investigation into the controller where
WebPushSubscriptionis used indicates that thesubscriptionobject received fromreq.bodyis not validated before being used. This supports the original concern regarding the lack of validation for thesubscriptionobject before it's passed to theWebPushDAO.upsertmethod. Therefore, the review comment is validated by the evidence found.* 46-53: Ensure that error handling for `webpush.sendNotification` is robust, especially considering network issues or third-party service failures. Consider retry mechanisms or logging for failed notification attempts to improve reliability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if there's a validation step for WebPushSubscription before it's used in the create method. rg "WebPushSubscription" --type tsLength of output: 1196
Script:
#!/bin/bash # Check for validation logic in the controller where WebPushSubscription is used. rg "WebPushSubscription" src/components/web-push/controller.tsLength of output: 182
src/config/errors.ts (1)
- 34-38: The error code
envis reused for multiple errors (KEY_NOT_FOUND,AGENT_PASS_NOT_SET,ALQ_URL_NOT_FOUND,ALQ_AUTH_NOT_FOUND). Consider using unique error codes for each scenario to improve error traceability and handling..gitignore (1)
- 107-120: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-120]
The additions to
.gitignoreare appropriate for enhancing security by excluding sensitive and unnecessary files from version control. Ensure that all team members are aware of these changes and adjust their local development environments accordingly.src/db/token.ts (2)
- 12-20: Repeatedly calling
prisma.$disconnect()in each method can lead to performance issues due to the overhead of establishing new database connections. Consider managing the Prisma client lifecycle at the application level instead.- 54-66: The
authorizeRevocationmethod throws a genericNotFoundExceptionwithout specifying what was not found. Providing more context in the error message, such as "Token not found", can improve debugging and user feedback.src/helpers/loggers.ts (1)
- 72-72: Logging the error object directly with
console.errorin theexitLogfunction is a good practice for debugging. However, ensure sensitive information is not logged in production environments.src/components/bank-accounts/validators.ts (1)
- 3-50: The validation schemas for bank account operations are comprehensive and correctly use
express-validatorto ensure the integrity of incoming data. Ensure that all fields validated here are also properly sanitized and validated in the business logic layer to prevent security vulnerabilities.src/db/seed.ts (1)
- 12-76: The seeding script correctly initializes required roles and creates a root user based on interactive input. Ensure that passwords are securely handled and stored, and consider implementing additional checks to prevent accidental execution in production environments.
__tests__/home.test.ts (1)
- 15-26: The test for the home endpoint correctly checks the response structure and status code. However, consider uncommenting and updating the commented-out tests to ensure comprehensive coverage of the endpoint's functionality, especially if the commented-out features are still relevant.
src/components/bank-accounts/controller.ts (3)
- 26-38: The implementation of the
createmethod looks good and follows best practices for error handling and service usage.- 44-44: Consider validating
account_idbefore converting it to a number to ensure it's a valid integer. This can prevent potential errors when invalid IDs are passed in the request.- 62-62: Consider validating
account_idbefore converting it to a number to ensure it's a valid integer. This can prevent potential errors when invalid IDs are passed in the request.src/chatbot/flows.ts (3)
- 14-22: The use of
setTimeoutandclearTimeoutfor managing bot idle timeout is a good approach. However, ensure thattimeoutIdis properly managed across different instances of the bot to avoid potential memory leaks or unexpected behavior.Consider encapsulating timeout management in a separate utility or class to improve maintainability and reduce complexity in the main flow functions.
- 30-41: The
submenuOptionHandlerfunction correctly handles user choices, including navigating to the main menu or specific flows based on the choice. However, it's important to validatectx.bodybefore converting it to a number to avoid potential runtime errors.- 44-51: Creating submenus using
createSubMenuis a neat approach. Ensure that themenuandanswersarrays are always properly formatted and contain the expected data to avoid runtime errors.Verification successful
Creating submenus using
createSubMenuis a neat approach. Themenuandanswersarrays are sourced both statically fromsrc/chatbot/messages.tsand dynamically from a database insrc/chatbot/flows.ts. The static source likely ensures a consistent format and data expectation, while the dynamic source introduces flexibility but also requires adherence to a specific structure, as indicated by the type casting.Ensure that the
menuandanswersarrays, whether sourced statically or dynamically, are always properly formatted and contain the expected data to avoid runtime errors. This might involve implementing additional validations for the dynamically sourced data to ensure consistency with the expected structure.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Placeholder for manual verification of menu and answers array formats. echo "Manually verify that menu and answers arrays are correctly formatted."Length of output: 148
Script:
#!/bin/bash # Search for instances where `createSubMenu` is called to examine the construction of `menu` and `answers` arrays. rg "createSubMenu" --context 5Length of output: 1061
Script:
#!/bin/bash # Examine the initialization and structure of `botMenus` and `botMessages`. rg "botMenus|botMessages" --context 5Length of output: 3386
src/components/agent/services.ts (4)
- 19-38: The
loginfunction correctly authenticates agents and generates token pairs. Ensure that sensitive information, such as passwords, is never logged or exposed unintentionally.Consider adding more detailed error messages for different failure cases to improve debugging and user feedback without compromising security.
- 41-43: The
showPaymentsfunction retrieves payments without any filtering or pagination. For large datasets, this could lead to performance issues.Consider implementing pagination or filtering options to improve performance and usability.
- 49-53: The
markAsPaidfunction updates the payment status. Ensure that the payment ID provided is validated and that the user has the necessary permissions to update the payment status.Verification successful
To verify if the
markAsPaidfunction includes validation for the payment ID and checks for user permissions, we need to inspect the code surrounding this function and any related components. We'll look for validation logic, permission checks, and howmarkAsPaidis called within the codebase.
The review comment's concerns about ensuring that the payment ID is validated and that user permissions are checked before updating the payment status are addressed through middleware functions (
validatePaymentIndex(),checkExact(), andthrowIfBadRequest) applied in the route definition before callingAgentController.markAsPaid. These middleware functions likely perform the necessary validation and permission checks. Therefore, the implementation aligns with the best practices of handling such checks outside the core business logic function, ensuring that only valid and authorized requests proceed to update the payment status.* 91-105: The `completePendingDeposits` function iterates over deposits and attempts to complete them. Ensure that transactions are handled correctly to avoid partial updates in case of errors.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Placeholder for manual verification of payment ID validation and user permissions. echo "Manually verify that payment ID validation and user permissions checks are implemented."Length of output: 184
Script:
#!/bin/bash # Search for the usage of markAsPaid to understand its context and check for validation and permissions rg "markAsPaid" --context 5 # Additionally, search for any explicit validation or permission checks in the file where markAsPaid is defined rg "validate|permission" src/components/agent/services.ts --context 5Length of output: 2623
Consider wrapping the deposit completion logic in a transaction to ensure atomicity and consistency.
prisma/migrations/20240220155527_initial_migration/migration.sql (3)
- 2-16: The
USERS_ROOTtable is well-defined with appropriate data types and constraints. Ensure that thepasswordfield is sufficiently large to store hashed passwords securely.Consider increasing the size of the
passwordfield if hashed passwords may exceed the current length limit.
- 33-53: The
PLAYERStable is designed to store player information. Ensure that sensitive information likepasswordis stored securely and consider adding indexes on frequently queried fields for performance optimization.Consider adding a comment about the need for secure password storage and the use of indexes for optimization.
- 95-107: Foreign key constraints are correctly added to ensure data integrity. However, ensure that the
ON DELETEandON UPDATEactions align with the application's data management policies.src/components/auth/services.ts (4)
- 12-35: The
tokensfunction generates access and refresh tokens. Ensure that tokens are generated securely and consider adding logging for monitoring and debugging purposes, without exposing sensitive information.Consider adding secure logging to monitor token generation without compromising security.
- 71-97: The JWT strategy configuration is correctly set up. However, ensure that the user agent is validated securely and consider handling cases where the token might be used from different devices by the same user.
Consider adding a mechanism to allow users to manage their active sessions and tokens, providing more flexibility and security.
- 103-105: The
invalidateTokensByUserAgentfunction invalidates tokens based on the user agent. Ensure that this does not inadvertently log out users from other devices unintentionally.- 117-130: The
logoutfunction attempts to invalidate the token and its children. Ensure that the token revocation process is secure and consider adding more detailed error handling for different types of JWT errors.Consider enhancing error handling in the logout process to provide clearer feedback for different JWT error types.
src/components/players/services.ts (2)
- 22-26: The
getPlayerByIdfunction retrieves player information securely by hiding the password. Ensure that the player ID is validated to prevent unauthorized access to other players' information.- 78-100: The
loginfunction correctly handles player login, including fallback to an external API if the player is not found locally. Ensure that sensitive information, such as passwords, is never logged or exposed unintentionally.Consider adding more detailed error messages for different failure cases to improve debugging and user feedback without compromising security.
src/middlewares/errorHandler.ts (4)
- 46-52: The addition of a Prisma error handler is a good practice for handling database-specific errors gracefully. Ensure that all possible Prisma error codes are covered and consider logging the errors for debugging purposes.
Consider adding logging for Prisma errors to aid in debugging while ensuring sensitive information is not exposed.
- 64-77: The
customErrorHandlerfunction correctly handles instances ofCustomError. Ensure that the error handling is comprehensive and covers all potential error cases that might be thrown within the application.- 80-91: The
CustomErrorclass is well-implemented for creating custom error objects. Ensure that the error codes and descriptions are meaningful and consistent across the application to improve maintainability and debugging.Consider creating a centralized error code and description management system to ensure consistency and ease of maintenance.
- 94-169: The
prismaErrorHandlerfunction provides detailed handling for various Prisma error codes. Ensure that the error messages are user-friendly and do not expose sensitive information or implementation details to the end user.Consider reviewing and refining the error messages to ensure they are user-friendly and secure.
src/db/deposits.ts (3)
- 30-52: The
indexmethod retrieves deposits with optional filtering. Consider implementing pagination to handle large datasets efficiently and improve performance.Consider adding pagination support to the
indexmethod to enhance scalability and user experience.
- 55-65: The
getByIdmethod retrieves a specific deposit by ID. Ensure that access control checks are in place to prevent unauthorized access to deposit information.Verification successful
The search results indicate multiple occurrences of
getByIdmethod calls across different files, includingsrc/db/token.ts,src/db/players.ts,src/db/deposits.ts,src/components/players/services.ts,src/components/auth/services.ts, andsrc/components/transactions/controller.ts. However, these results also include calls togetByIdmethods for tokens and players, not just deposits.The relevant findings for deposit access control are within
src/db/deposits.tsandsrc/components/transactions/controller.ts. Specifically, theauthorizeTransactionmethod insrc/db/deposits.tsusesgetByIdto retrieve a deposit and then performs checks to ensure the deposit belongs to the correct player, throwing aNotFoundExceptionorForbiddenErrorif these checks fail. This indicates that at least in this context, there are access control checks in place for retrieving deposit information.Given this information, it can be concluded that there are mechanisms to ensure access control for deposit retrieval, at least in the context of authorizing transactions. However, without a comprehensive review of every call to
getByIdand the surrounding code, it's not possible to guarantee that all access to deposit information is properly secured across the entire codebase.Considering the evidence found in the
authorizeTransactionmethod, it appears that the review comment's concern about ensuring access control checks for deposit information retrieval has been addressed in at least one part of the codebase. Therefore, the verification process can be concluded with the finding that access control checks are implemented for deposit retrieval in certain contexts.* 89-99: The `getPendingCoinTransfers` method retrieves deposits pending coin transfers. Ensure that this method includes appropriate error handling and consider adding logging for monitoring purposes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Placeholder for manual verification of access control checks. echo "Manually verify that access control checks are implemented for deposit retrieval."Length of output: 172
Script:
#!/bin/bash # Searching for calls to the getById method within the codebase rg "getById" --context 5Length of output: 6938
Consider adding error handling and logging to the
getPendingCoinTransfersmethod to improve reliability and observability.__tests__/bank-account.test.ts (2)
- 16-18: Ensure that the
initializeandcleanUpfunctions properly set up and tear down the test environment, including database connections and any necessary data setup or cleanup. Consider adding explicit checks or comments detailing what these functions do for clarity.- 20-140: Overall, the test cases are well-structured and cover a broad range of scenarios for bank account operations. However, consider adding more detailed assertions for the response body in tests where only the status code is checked. This will ensure that the API not only returns the correct status but also the expected data structure and content.
__tests__/player.test.ts (2)
- 10-13: The setup method correctly initializes the Prisma client and the test agent. Ensure that any global test data or configurations are also initialized here if needed.
- 15-181: The test cases cover a wide range of scenarios for player operations. To further improve test coverage, consider adding tests for edge cases such as invalid input formats (e.g., email format validation) and testing the actual content of the response body in success cases to ensure the API returns the expected data.
__tests__/auth.test.ts (2)
- 26-27: The
beforeAllandafterAllmethods are used for setup and cleanup, respectively. Ensure that these methods adequately prepare the test environment and clean up any resources or data to prevent side effects between tests.- 29-138: The test cases comprehensively cover the authentication flow, including token refresh and logout scenarios. To enhance these tests, consider verifying the actual content of the refreshed tokens in the success cases to ensure they meet the expected format and validity. Additionally, for the logout tests, verifying that the token is indeed invalidated (e.g., by attempting to use it after logout) would strengthen the test suite.
__tests__/web-push.test.ts (2)
- 22-23: The
beforeAllandafterAllmethods are used for setup and cleanup, respectively. It's important to ensure that these methods properly initialize and clean up the test environment, including any mock data or configurations specific to web push notifications.- 25-159: The test cases effectively cover the main operations related to web push subscriptions. To further improve the robustness of these tests, consider adding assertions to verify the actual content of the response body in success cases, ensuring that the API returns the expected subscription details. Additionally, for the deletion tests, verifying that the subscription is indeed removed (e.g., by attempting to fetch or use the deleted subscription) would provide stronger validation.
src/components/transactions/services.ts (1)
- 26-330: The implementation of transaction services in this file appears comprehensive and covers a wide range of functionalities including deposits, cashouts, and verification of payments. A few suggestions for improvement:
- Ensure that all external API calls and database operations are properly error-handled to prevent unhandled exceptions from crashing the application.
- Consider adding more detailed logging for critical operations to aid in debugging and monitoring.
- Review the use of
anyin type annotations (e.g., inlogTransactionmethod) and replace with more specific types where possible to improve type safety.src/chatbot/messages.ts (1)
- 1-292: Consider implementing a more dynamic approach for managing chatbot messages, such as a CMS or a database-driven solution. This would allow for easier updates and localization without requiring code changes. Ensure the content of the messages is reviewed for accuracy and does not inadvertently disclose sensitive information.
__tests__/transactions.test.ts (1)
- 306-322: Consider parameterizing hardcoded values such as currency codes to increase test flexibility and reduce potential maintenance issues if the application's requirements change.
README.md (3)
- 63-63: The use of
[🔒](#👉-🔒)to indicate a locked or secure endpoint is creative, but ensure this notation is clear to all readers. Consider adding a brief explanation at the beginning of the documentation on what the lock symbol signifies.Consider adding a brief explanation at the beginning of the documentation to clarify what the lock symbol signifies for endpoint security.
- 437-437: The deployment instructions mention using
npm run seedto register an agent in the database, which requires user and password inputs. Ensure there's a secure method for handling these credentials, especially in a production environment.Consider verifying and documenting the security measures for handling credentials during the seeding process.
- 441-458: The TODO section lists several tasks, including "Instanciar servicios en lugar de usar metodos estaticos" and "Cambiar contraseña". It's good practice to track these tasks in a project management tool or issue tracker rather than in the README.
Consider moving TODO items to a project management tool or issue tracker for better visibility and tracking.
__tests__/agent.test.ts (1)
- 409-453: The
initializefunction sets up test data for the agent tests, including creating a player with bank accounts, payments, and deposits. While this setup is comprehensive, consider the following improvements for maintainability and readability:
- Extracting the creation of test data into separate, well-named functions for clarity.
- Using constants for repeated literals like "MXN" or "Test Bank " to avoid magic strings and facilitate changes.
Consider refactoring the setup process to improve code readability and maintainability.
seed.sql (10)
- 23-27: The
INSERT INTO BANK_ACCOUNTSstatement is correctly formatted and uses appropriate data for initial seeding.- 29-30: Ensure the JSON structure inserted into
BOT_MESSAGESis correctly formatted and all URLs within the JSON are valid and point to the intended destinations.- 32-34: The
INSERT INTO DEPOSITSstatement is correctly formatted and uses appropriate data for initial seeding.- 36-60: The
INSERT INTO PAYMENTSstatement is correctly formatted and uses appropriate data for initial seeding.- 64-66: The
INSERT INTO ROLESstatement is correctly formatted and uses appropriate data for initial seeding.- 68-79: The
INSERT INTO TOKENSstatement is correctly formatted and uses appropriate data for initial seeding.- 81-92: The
INSERT INTO TRANSACTIONSstatement is correctly formatted and uses appropriate data for initial seeding.- 94-95: Verify the encryption method used for the password in
USERS_ROOTto ensure it meets security standards. Additionally, ensure the JSON structures are correctly formatted and contain valid data.- 97-98: The
INSERT INTO WEB_PUSH_SUBSCRIPTIONSstatement is correctly formatted and uses appropriate data for initial seeding.- 100-112: The
INSERT INTO _PlayerToRolestatement is correctly formatted and uses appropriate data for initial seeding.
| | ||
| /** Wrong token */ | ||
| it("Should return 401", async () => { | ||
| const response = await await agent |
There was a problem hiding this comment.
There's a duplicated await keyword in the asynchronous call, which is unnecessary and could potentially lead to confusion.
- const response = await await agent + const response = await agentCommittable 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.
Suggested change
| const response = await await agent | |
| const response = await agent |
Comment on lines +456 to +463
| async function cleanUp() { | ||
| await prisma.payment.delete({ where: { id: payment.id } }); | ||
| await prisma.deposit.delete({ where: { id: deposit.id } }); | ||
| await prisma.bankAccount.delete({ where: { id: player.BankAccounts[0].id } }); | ||
| await prisma.token.deleteMany({ where: { player_id: player.id } }); | ||
| // await prisma.token.delete({ where: { }}) | ||
| await prisma.player.delete({ where: { id: player.id } }); | ||
| prisma.$disconnect(); |
There was a problem hiding this comment.
The cleanUp function is responsible for cleaning up the test data after the tests have run. It's good practice to ensure that tests do not leave any residual data that could affect other tests. However, there's a commented-out line of code that seems to be a leftover from development. It's important to remove such commented-out code to keep the codebase clean and maintainable.
- // await prisma.token.delete({ where: { }})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.
Suggested change
| async function cleanUp() { | |
| await prisma.payment.delete({ where: { id: payment.id } }); | |
| await prisma.deposit.delete({ where: { id: deposit.id } }); | |
| await prisma.bankAccount.delete({ where: { id: player.BankAccounts[0].id } }); | |
| await prisma.token.deleteMany({ where: { player_id: player.id } }); | |
| // await prisma.token.delete({ where: { }}) | |
| await prisma.player.delete({ where: { id: player.id } }); | |
| prisma.$disconnect(); | |
| async function cleanUp() { | |
| await prisma.payment.delete({ where: { id: payment.id } }); | |
| await prisma.deposit.delete({ where: { id: deposit.id } }); | |
| await prisma.bankAccount.delete({ where: { id: player.BankAccounts[0].id } }); | |
| await prisma.token.deleteMany({ where: { player_id: player.id } }); | |
| await prisma.player.delete({ where: { id: player.id } }); | |
| prisma.$disconnect(); |
Comment on lines +342 to +352
| async function cleanUp() { | ||
| await prisma.token.deleteMany({ | ||
| where: { player_id: players[0].id, user_agent: USER_AGENT }, | ||
| }); | ||
| await prisma.token.deleteMany({ | ||
| where: { player_id: players[1].id, user_agent: USER_AGENT }, | ||
| }); | ||
| await prisma.deposit.delete({ where: { id: deposits[0].id } }); | ||
| // await prisma.deposit.delete({ where: { id: deposits[1].id .} }); | ||
| // await prisma.player.delete({ where: { id: player.id } }); | ||
| prisma.$disconnect(); |
There was a problem hiding this comment.
Ensure the cleanup function deletes all resources created during the tests to prevent test pollution. It appears that some deposits might not be deleted if the test execution is interrupted or if certain tests fail.
- // await prisma.deposit.delete({ where: { id: deposits[1].id .} }); + await prisma.deposit.deleteMany({ where: { OR: [{ id: deposits[0]?.id }, { id: deposits[1]?.id }] } });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.
Suggested change
| async function cleanUp() { | |
| await prisma.token.deleteMany({ | |
| where: { player_id: players[0].id, user_agent: USER_AGENT }, | |
| }); | |
| await prisma.token.deleteMany({ | |
| where: { player_id: players[1].id, user_agent: USER_AGENT }, | |
| }); | |
| await prisma.deposit.delete({ where: { id: deposits[0].id } }); | |
| // await prisma.deposit.delete({ where: { id: deposits[1].id .} }); | |
| // await prisma.player.delete({ where: { id: player.id } }); | |
| prisma.$disconnect(); | |
| async function cleanUp() { | |
| await prisma.token.deleteMany({ | |
| where: { player_id: players[0].id, user_agent: USER_AGENT }, | |
| }); | |
| await prisma.token.deleteMany({ | |
| where: { player_id: players[1].id, user_agent: USER_AGENT }, | |
| }); | |
| await prisma.deposit.delete({ where: { id: deposits[0].id } }); | |
| await prisma.deposit.deleteMany({ where: { OR: [{ id: deposits[0]?.id }, { id: deposits[1]?.id }] } }); | |
| // await prisma.player.delete({ where: { id: player.id } }); | |
| prisma.$disconnect(); |
Comment on lines +3 to +8
| SET NAMES utf8; | ||
| SET time_zone = '+00:00'; | ||
| SET foreign_key_checks = 0; | ||
| SET sql_mode = 'NO_AUTO_VALUE_ON_ZERO'; | ||
| | ||
| SET NAMES utf8mb4; |
There was a problem hiding this comment.
The SET NAMES utf8; statement on line 3 is immediately overridden by SET NAMES utf8mb4; on line 8. Consider removing the first SET NAMES statement to avoid confusion and ensure clarity in character set specification.
- SET NAMES utf8;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.
Suggested change
| SET NAMES utf8; | |
| SET time_zone = '+00:00'; | |
| SET foreign_key_checks = 0; | |
| SET sql_mode = 'NO_AUTO_VALUE_ON_ZERO'; | |
| SET NAMES utf8mb4; | |
| SET time_zone = '+00:00'; | |
| SET foreign_key_checks = 0; | |
| SET sql_mode = 'NO_AUTO_VALUE_ON_ZERO'; | |
| SET NAMES utf8mb4; |
Comment on lines +9 to +12
| INSERT INTO `PLAYERS` (`id`, `panel_id`, `username`, `password`, `email`, `first_name`, `last_name`, `date_of_birth`, `movile_number`, `country`, `balance_currency`, `status`, `created_at`, `updated_at`) VALUES | ||
| (1, 3900, 'test19', '$2b$10$4ReAbWcT.Q8PLGO2Gkc6H.qFTJabPC.cgPDAPfclAt1/ssLWMwh52', 'hello@example.com', NULL, NULL, NULL, NULL, NULL, 'MX', 'ACTIVO', '2024-02-02 16:01:19.264', '2024-03-06 17:05:28.401'), | ||
| (2, 3859, 'test17', '$2b$10$twY7L8HNkIlYqFIOg5I9u.gluleI55wFGZ5L.iyw4SNISlJrIH9iy', 'bye@example.com', NULL, NULL, NULL, NULL, NULL, 'MX', 'ACTIVO', '2024-02-06 11:45:04.408', '2024-03-06 17:06:24.766'), | ||
| (3, 3940, 'test20', '03ac674216f3e15c761ee1a5e255f067953623c8b388b4459e13f978d7c846f4', 'me@example.com', NULL, NULL, NULL, NULL, NULL, 'MXN', 'ACTIVO', '2024-02-09 19:57:41.941', '2024-02-09 19:57:41.941'), |
There was a problem hiding this comment.
Storing passwords in plain text (as seen in some entries) poses a significant security risk. Ensure all passwords are securely hashed before storage to protect user information.
Comment on lines +75 to +88
| static async getBalance(): Promise<BalanceResponse> { | ||
| const url = "accounts/user"; | ||
| const httpService = new HttpService(); | ||
| const response = await httpService.authedAgentApi.get(url); | ||
| if (response.status !== 200) | ||
| throw new CustomError({ | ||
| code: "error_balance", | ||
| status: response.status, | ||
| description: "Error en el panel al obtener el balance", | ||
| }); | ||
| return { | ||
| balance: Number(response.data.balance), | ||
| currency: response.data.balance_currency, | ||
| }; |
There was a problem hiding this comment.
The getBalance function makes an external API call to retrieve the balance. Ensure that error handling is robust, especially for handling different response statuses and potential network issues.
const response = await httpService.authedAgentApi.get(url); + try { + const response = await httpService.authedAgentApi.get(url); + } catch (error) { + console.error("Failed to retrieve balance:", error); + throw new CustomError({ + code: "balance_retrieval_failed", + description: "Failed to retrieve balance due to an external API error", + }); + } if (response.status !== 200)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.
Suggested change
| static async getBalance(): Promise<BalanceResponse> { | |
| const url = "accounts/user"; | |
| const httpService = new HttpService(); | |
| const response = await httpService.authedAgentApi.get(url); | |
| if (response.status !== 200) | |
| throw new CustomError({ | |
| code: "error_balance", | |
| status: response.status, | |
| description: "Error en el panel al obtener el balance", | |
| }); | |
| return { | |
| balance: Number(response.data.balance), | |
| currency: response.data.balance_currency, | |
| }; | |
| static async getBalance(): Promise<BalanceResponse> { | |
| const url = "accounts/user"; | |
| const httpService = new HttpService(); | |
| try { | |
| const response = await httpService.authedAgentApi.get(url); | |
| if (response.status !== 200) | |
| throw new CustomError({ | |
| code: "error_balance", | |
| status: response.status, | |
| description: "Error en el panel al obtener el balance", | |
| }); | |
| return { | |
| balance: Number(response.data.balance), | |
| currency: response.data.balance_currency, | |
| }; | |
| } catch (error) { | |
| console.error("Failed to retrieve balance:", error); | |
| throw new CustomError({ | |
| code: "balance_retrieval_failed", | |
| description: "Failed to retrieve balance due to an external API error", | |
| }); | |
| } |
Comment on lines +19 to +30
| CREATE TABLE `TRANSACTIONS` ( | ||
| `id` INTEGER NOT NULL AUTO_INCREMENT, | ||
| `sender_id` INTEGER NOT NULL, | ||
| `recipient_id` INTEGER NOT NULL, | ||
| `amount` DOUBLE NOT NULL, | ||
| `date` DATETIME(3) NOT NULL, | ||
| `status` VARCHAR(100) NOT NULL, | ||
| `created_at` DATETIME(3) NOT NULL DEFAULT CURRENT_TIMESTAMP(3), | ||
| `updated_at` DATETIME(3) NOT NULL, | ||
| | ||
| PRIMARY KEY (`id`) | ||
| ) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; |
There was a problem hiding this comment.
The TRANSACTIONS table includes essential fields for tracking transactions. Consider adding foreign key constraints to sender_id and recipient_id to ensure referential integrity with the PLAYERS table.
ALTER TABLE `TRANSACTIONS` ADD CONSTRAINT `fk_sender_id` FOREIGN KEY (`sender_id`) REFERENCES `PLAYERS`(`panel_id`); ALTER TABLE `TRANSACTIONS` ADD CONSTRAINT `fk_recipient_id` FOREIGN KEY (`recipient_id`) REFERENCES `PLAYERS`(`panel_id`); Comment on lines +41 to +64
| async refresh(refresh: string, user_agent?: string): Promise<TokenPair> { | ||
| if (!this.verifyTokenExpiration(refresh)) | ||
| throw new CustomError(ERR.TOKEN_EXPIRED); | ||
| | ||
| const payload = this.decodePayload(refresh); | ||
| if (payload.type !== "refresh") throw new CustomError(ERR.TOKEN_INVALID); | ||
| | ||
| let token = await TokenDAO.getById(payload.jti); | ||
| if (!token) throw new CustomError(ERR.TOKEN_INVALID); | ||
| | ||
| if (token.user_agent != user_agent) | ||
| token = await this.invalidateTokenById(token.id); | ||
| | ||
| if (token.invalid) { | ||
| await this.invalidateChildren(token); | ||
| // TODO | ||
| // notify("Uso duplicado de refresh token"); | ||
| throw new CustomError(ERR.TOKEN_INVALID); | ||
| } | ||
| | ||
| const { tokens, jti } = await this.tokens(payload.sub, user_agent); | ||
| // Invalidate received token and link it to newly created one. | ||
| await TokenDAO.updateById(token.id, { invalid: true, next: jti }); | ||
| return tokens; |
There was a problem hiding this comment.
The refresh function handles token refresh logic. Ensure that the refresh token is validated securely and that the old token is invalidated properly to prevent reuse.
if (token.user_agent != user_agent) + if (token.user_agent !== user_agent || token.invalid) token = await this.invalidateTokenById(token.id);Ensure that the token is also checked for invalidation before proceeding.
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.
Suggested change
| async refresh(refresh: string, user_agent?: string): Promise<TokenPair> { | |
| if (!this.verifyTokenExpiration(refresh)) | |
| throw new CustomError(ERR.TOKEN_EXPIRED); | |
| const payload = this.decodePayload(refresh); | |
| if (payload.type !== "refresh") throw new CustomError(ERR.TOKEN_INVALID); | |
| let token = await TokenDAO.getById(payload.jti); | |
| if (!token) throw new CustomError(ERR.TOKEN_INVALID); | |
| if (token.user_agent != user_agent) | |
| token = await this.invalidateTokenById(token.id); | |
| if (token.invalid) { | |
| await this.invalidateChildren(token); | |
| // TODO | |
| // notify("Uso duplicado de refresh token"); | |
| throw new CustomError(ERR.TOKEN_INVALID); | |
| } | |
| const { tokens, jti } = await this.tokens(payload.sub, user_agent); | |
| // Invalidate received token and link it to newly created one. | |
| await TokenDAO.updateById(token.id, { invalid: true, next: jti }); | |
| return tokens; | |
| async refresh(refresh: string, user_agent?: string): Promise<TokenPair> { | |
| if (!this.verifyTokenExpiration(refresh)) | |
| throw new CustomError(ERR.TOKEN_EXPIRED); | |
| const payload = this.decodePayload(refresh); | |
| if (payload.type !== "refresh") throw new CustomError(ERR.TOKEN_INVALID); | |
| let token = await TokenDAO.getById(payload.jti); | |
| if (!token) throw new CustomError(ERR.TOKEN_INVALID); | |
| if (token.user_agent !== user_agent || token.invalid) | |
| token = await this.invalidateTokenById(token.id); | |
| if (token.invalid) { | |
| await this.invalidateChildren(token); | |
| // TODO | |
| // notify("Uso duplicado de refresh token"); | |
| throw new CustomError(ERR.TOKEN_INVALID); | |
| } | |
| const { tokens, jti } = await this.tokens(payload.sub, user_agent); | |
| // Invalidate received token and link it to newly created one. | |
| await TokenDAO.updateById(token.id, { invalid: true, next: jti }); | |
| return tokens; |
Comment on lines +33 to +72
| create = async (player: PlayerRequest): Promise<PlainPlayerResponse> => { | ||
| const panelSignUpUrl = "/pyramid/create/player/"; | ||
| const playerLoginUrl = "/accounts/login/"; | ||
| const { authedAgentApi, playerApi } = new HttpService(); | ||
| | ||
| // Crear el usuario en panel | ||
| let response = await authedAgentApi.post(panelSignUpUrl, player); | ||
| | ||
| if (response.status !== 201 && response.status !== 400) { | ||
| throw new CustomError({ | ||
| status: 500, | ||
| code: "error_panel", | ||
| description: "Error en el panel al crear el usuario", | ||
| }); | ||
| } | ||
| | ||
| if ( | ||
| response.status === 400 && | ||
| (response.data.code === "already_exists" || | ||
| response.data.code === "user_already_exists") | ||
| ) { | ||
| // Usuario existe en el panel, verificar que esté en local | ||
| const localPlayer = await PlayersDAO.getByUsername(player.username); | ||
| | ||
| // Usuario existe en panel y en local. Devolver "Ya existe" | ||
| if (localPlayer) throw new CustomError(ERR.USER_ALREADY_EXISTS); | ||
| | ||
| // Usuario existe en panel pero no en local, loguearlo para obtener | ||
| // su panel_id | ||
| response = await playerApi.post(playerLoginUrl, player); | ||
| // Credenciales inválidas | ||
| if (response.status !== 200) | ||
| throw new CustomError(ERR.USER_ALREADY_EXISTS); | ||
| } | ||
| | ||
| // Crear usuario en local | ||
| player.panel_id = response.data.id; | ||
| player.password = await hash(player.password); | ||
| const localPlayer = await PlayersDAO.create(player); | ||
| return hidePassword(localPlayer); |
There was a problem hiding this comment.
The create function handles player creation, including interactions with external APIs. Ensure that error handling is robust, especially for handling API response statuses and potential network issues.
let response = await authedAgentApi.post(panelSignUpUrl, player); + try { + let response = await authedAgentApi.post(panelSignUpUrl, player); + } catch (error) { + console.error("Failed to create player in panel:", error); + throw new CustomError({ + code: "player_creation_failed", + description: "Failed to create player due to an external API error", + }); + } if (response.status !== 201 && response.status !== 400)Ensure that external API interactions are wrapped in try-catch blocks for robust error handling.
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.
Suggested change
| create = async (player: PlayerRequest): Promise<PlainPlayerResponse> => { | |
| const panelSignUpUrl = "/pyramid/create/player/"; | |
| const playerLoginUrl = "/accounts/login/"; | |
| const { authedAgentApi, playerApi } = new HttpService(); | |
| // Crear el usuario en panel | |
| let response = await authedAgentApi.post(panelSignUpUrl, player); | |
| if (response.status !== 201 && response.status !== 400) { | |
| throw new CustomError({ | |
| status: 500, | |
| code: "error_panel", | |
| description: "Error en el panel al crear el usuario", | |
| }); | |
| } | |
| if ( | |
| response.status === 400 && | |
| (response.data.code === "already_exists" || | |
| response.data.code === "user_already_exists") | |
| ) { | |
| // Usuario existe en el panel, verificar que esté en local | |
| const localPlayer = await PlayersDAO.getByUsername(player.username); | |
| // Usuario existe en panel y en local. Devolver "Ya existe" | |
| if (localPlayer) throw new CustomError(ERR.USER_ALREADY_EXISTS); | |
| // Usuario existe en panel pero no en local, loguearlo para obtener | |
| // su panel_id | |
| response = await playerApi.post(playerLoginUrl, player); | |
| // Credenciales inválidas | |
| if (response.status !== 200) | |
| throw new CustomError(ERR.USER_ALREADY_EXISTS); | |
| } | |
| // Crear usuario en local | |
| player.panel_id = response.data.id; | |
| player.password = await hash(player.password); | |
| const localPlayer = await PlayersDAO.create(player); | |
| return hidePassword(localPlayer); | |
| create = async (player: PlayerRequest): Promise<PlainPlayerResponse> => { | |
| const panelSignUpUrl = "/pyramid/create/player/"; | |
| const playerLoginUrl = "/accounts/login/"; | |
| const { authedAgentApi, playerApi } = new HttpService(); | |
| // Crear el usuario en panel | |
| try { | |
| let response = await authedAgentApi.post(panelSignUpUrl, player); | |
| } catch (error) { | |
| console.error("Failed to create player in panel:", error); | |
| throw new CustomError({ | |
| code: "player_creation_failed", | |
| description: "Failed to create player due to an external API error", | |
| }); | |
| } | |
| if (response.status !== 201 && response.status !== 400) { | |
| throw new CustomError({ | |
| status: 500, | |
| code: "error_panel", | |
| description: "Error en el panel al crear el usuario", | |
| }); | |
| } | |
| if ( | |
| response.status === 400 && | |
| (response.data.code === "already_exists" || | |
| response.data.code === "user_already_exists") | |
| ) { | |
| // Usuario existe en el panel, verificar que esté en local | |
| const localPlayer = await PlayersDAO.getByUsername(player.username); | |
| // Usuario existe en panel y en local. Devolver "Ya existe" | |
| if (localPlayer) throw new CustomError(ERR.USER_ALREADY_EXISTS); | |
| // Usuario existe en panel pero no en local, loguearlo para obtener | |
| // su panel_id | |
| response = await playerApi.post(playerLoginUrl, player); | |
| // Credenciales inválidas | |
| if (response.status !== 200) | |
| throw new CustomError(ERR.USER_ALREADY_EXISTS); | |
| } | |
| // Crear usuario en local | |
| player.panel_id = response.data.id; | |
| player.password = await hash(player.password); | |
| const localPlayer = await PlayersDAO.create(player); | |
| return hidePassword(localPlayer); |
Comment on lines +17 to +28
| static create(data: CreateDepositProps): Promise<Deposit> { | ||
| try { | ||
| return prisma.deposit.create({ | ||
| data: { ...data, status: CONFIG.SD.DEPOSIT_STATUS.PENDING }, | ||
| // include: { Player: true }, | ||
| }); | ||
| } catch (error) { | ||
| throw error; | ||
| } finally { | ||
| prisma.$disconnect(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The create method correctly creates a deposit entry in the database. Ensure that the method properly handles potential exceptions from the Prisma client to avoid unhandled promise rejections.
try { + return await prisma.deposit.create({ data: { ...data, status: CONFIG.SD.DEPOSIT_STATUS.PENDING }, // include: { Player: true }, }); - } catch (error) { - throw error; - } finally { - prisma.$disconnect(); - }Consider removing the unnecessary try-catch block and ensuring proper resource management.
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.
Suggested change
| static create(data: CreateDepositProps): Promise<Deposit> { | |
| try { | |
| return prisma.deposit.create({ | |
| data: { ...data, status: CONFIG.SD.DEPOSIT_STATUS.PENDING }, | |
| // include: { Player: true }, | |
| }); | |
| } catch (error) { | |
| throw error; | |
| } finally { | |
| prisma.$disconnect(); | |
| } | |
| } | |
| static create(data: CreateDepositProps): Promise<Deposit> { | |
| return await prisma.deposit.create({ | |
| data: { ...data, status: CONFIG.SD.DEPOSIT_STATUS.PENDING }, | |
| // include: { Player: true }, | |
| }); | |
| } |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
.gitignoreto exclude environment-specific files and temporary data.