♻️ Capitalcom _fetch_account_data refactor #481
Conversation
Reviewer's Guide by SourceryThis pull request refactors the _fetch_account_data method in cefi/handler/capitalcom.py to enhance error handling and logging, and improve the logic for setting the account number. Additionally, a new test case for modifying positions is added in tests/test_unit.py, with some assertions commented out for future validation. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mraniki - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
Here's what I looked at during the review
- 🔴 General issues: 2 blocking issues, 2 other issues
- 🟡 Security: 1 issue found
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| # try: | ||
| # if self.default_account: | ||
| # logger.debug("Default account: {}", self.default_account) | ||
| # self.switch_account(self.default_account) | ||
| # self.accounts_data = self.client.all_accounts() | ||
| # except Exception as e: | ||
| # logger.error("Error fetching account data: {}", e) | ||
| # return False | ||
| # logger.debug("Account data: {}", self.accounts_data) | ||
| # self.account_number = self.accounts_data["accounts"][0]["accountId"] |
There was a problem hiding this comment.
issue: Remove commented-out code
The commented-out code should be removed to keep the codebase clean and maintainable. If this code is no longer needed, it's better to delete it rather than leaving it commented out.
| try: | ||
| self.accounts_data = self.client.all_accounts() | ||
| logger.debug("Account data: {}", self.accounts_data) |
There was a problem hiding this comment.
🚨 issue (security): Potential sensitive data logging
Ensure that self.accounts_data does not contain sensitive information before logging it. Logging sensitive data can lead to security vulnerabilities.
| for account in self.accounts_data["accounts"]: | ||
| if account["accountId"] == self.default_account: | ||
| self.account_number = account["accountId"] |
There was a problem hiding this comment.
suggestion (performance): Inefficient loop for finding default account
Consider using a more efficient method to find the default account, such as a dictionary lookup, if possible. This will improve the performance, especially if the number of accounts is large.
| for account in self.accounts_data["accounts"]: | |
| if account["accountId"] == self.default_account: | |
| self.account_number = account["accountId"] | |
| account_dict = {account["accountId"]: account for account in self.accounts_data["accounts"]} | |
| self.account_number = account_dict.get(self.default_account, self.accounts_data["accounts"][0])["accountId"] |
| self.account_number = self.accounts_data["accounts"][0]["accountId"] | ||
|
|
||
| logger.debug("Account number: {}", self.account_number) | ||
| logger.debug("Session details: {}", self.client.get_sesion_details()) |
There was a problem hiding this comment.
issue (typo): Typo in method name
There is a typo in the method name get_sesion_details(). It should be get_session_details(). Ensure that the correct method name is used.
| @@ -106,7 +123,9 @@ def switch_account(self, account_number): | |||
| Returns: | |||
| None | |||
| """ | |||
| self.client.switch_account(self.default_account) | |||
| logger.debug("Switching to account {}", account_number) | |||
| switch = self.client.switch_account(self.default_account) | |||
There was a problem hiding this comment.
issue (bug_risk): Incorrect variable used in switch_account
The method switch_account should use the account_number parameter instead of self.default_account. This ensures that the correct account is switched to.
| @pytest.mark.asyncio | ||
| async def test_modify_position(CXTrader, order): | ||
| result = await CXTrader.modify_position(order) | ||
| print(result) | ||
| assert result is not None | ||
|
|
||
| # assert ("binance" in result) or ("huobi" in result) or ("capital" in result) | ||
| # assert ("🔵" in result) or ("Error" in result) |
There was a problem hiding this comment.
suggestion (testing): Add more assertions to cover edge cases
The test test_modify_position currently only checks if the result is not None. It would be beneficial to add more assertions to cover different edge cases and expected outcomes. For example, you could check if the result contains specific keys or values that indicate a successful modification or an error.
| @pytest.mark.asyncio | |
| async def test_modify_position(CXTrader, order): | |
| result = await CXTrader.modify_position(order) | |
| print(result) | |
| assert result is not None | |
| # assert ("binance" in result) or ("huobi" in result) or ("capital" in result) | |
| # assert ("🔵" in result) or ("Error" in result) | |
| @pytest.mark.asyncio | |
| async def test_modify_position(CXTrader, order): | |
| result = await CXTrader.modify_position(order) | |
| print(result) | |
| assert result is not None | |
| assert isinstance(result, dict) | |
| assert "status" in result | |
| assert result["status"] in ["success", "error"] | |
| if result["status"] == "error": | |
| assert "message" in result |
| async def test_modify_position(CXTrader, order): | ||
| result = await CXTrader.modify_position(order) | ||
| print(result) | ||
| assert result is not None |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for exception handling
It would be useful to add a test case that simulates an exception being raised during the modify_position method. This will ensure that your code handles exceptions gracefully and behaves as expected under error conditions.
| async def test_modify_position(CXTrader, order): | |
| result = await CXTrader.modify_position(order) | |
| print(result) | |
| assert result is not None | |
| async def test_modify_position(CXTrader, order): | |
| try: | |
| result = await CXTrader.modify_position(order) | |
| print(result) | |
| assert result is not None | |
| except Exception as e: | |
| print(f"Exception occurred: {e}") | |
| assert False |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
===========================================
+ Coverage 58.69% 74.19% +15.49%
===========================================
Files 7 7
Lines 460 465 +5
===========================================
+ Hits 270 345 +75
+ Misses 190 120 -70 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request refactors the _fetch_account_data method in the capitalcom handler to enhance account switching logic and error handling. Additionally, a new test case for the modify_position method has been added.