Fix: Removing condition for bank account in sync data #179
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request modifies several components in the codebase, including the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/business_central/utils.py (2)
Line range hint
507-521: Fix undefined variable bug in post_attachments methodThe method returns
post_responsewhich is only defined within theifblock. If there are no attachments, this would result in an undefined variable error.Apply this fix:
def post_attachments( self, ref_type: str, ref_id: str, attachments: List[Dict] ) -> List: """ Link attachments to objects Business Central :param ref_id: object id :param ref_type: type of object :param attachments: attachment[dict()] """ responses = [] + post_response = None if len(attachments): for attachment in attachments: data = { "parentId": ref_id, "fileName": "{0}_{1}".format(attachment["id"], attachment["name"]), "parentType": ref_type } post_response = self.connection.attachments.post(data) self.connection.attachments.upload( post_response["id"], attachment["content_type"], base64.b64decode(attachment["download_url"]) ) responses.append(post_response) - return post_response + return responsesAlso note that the method's docstring indicates it returns a List, but it's currently returning a single response. The fix above corrects this by returning the
responseslist.
Line range hint
507-521: Add error handling for API callsThe post_attachments method should handle potential API errors to prevent failures from affecting the entire sync process.
Consider adding try-catch blocks:
def post_attachments( self, ref_type: str, ref_id: str, attachments: List[Dict] ) -> List: """ Link attachments to objects Business Central :param ref_id: object id :param ref_type: type of object :param attachments: attachment[dict()] """ responses = [] for attachment in attachments: + try: data = { "parentId": ref_id, "fileName": "{0}_{1}".format(attachment["id"], attachment["name"]), "parentType": ref_type } post_response = self.connection.attachments.post(data) self.connection.attachments.upload( post_response["id"], attachment["content_type"], base64.b64decode(attachment["download_url"]) ) responses.append(post_response) + except Exception as e: + logger.error( + 'Failed to upload attachment %s: %s', + attachment.get('name', 'unknown'), + str(e) + ) + continue return responses
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/business_central/utils.py(1 hunks)
🔇 Additional comments (1)
apps/business_central/utils.py (1)
131-131: Verify the impact on bank account synchronization
The simplified logic for determining destination_id removes special handling for bank accounts. While the code is now more consistent, we should verify that this change doesn't break existing bank account synchronization.
✅ Verification successful
Let me gather more context about the bank account synchronization logic to make a definitive assessment.
Let me check the implementation of _sync_data method to understand how bank accounts were handled before and after the change.
Based on the gathered evidence, I can now provide the final response:
The simplified logic for bank account sync is safe to use
The change to use item['number'] if item.get('number') else item['id'] is actually an improvement because:
- Bank accounts are synced like other entities through the generic
_sync_datamethod - The test
test_sync_bank_accountsverifies the sync works correctly with the actual bank account data - The field 'number' is explicitly included in the
field_namesparameter when syncing bank accounts - The simplified logic maintains the same priority (number over id) while adding safer access using
.get()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for bank account references and their number/id usage
# Search for bank account related code and their handling
rg -A 5 "BANK_ACCOUNT" --type py
# Search for any tests related to bank account synchronization
rg -l "test.*bank.*account" --type py
Length of output: 2535
Script:
#!/bin/bash
# Check the test cases for bank account synchronization
cat tests/test_business_central/test_utils.py
# Look for the sync_bank_accounts implementation
ast-grep --pattern 'def sync_bank_accounts($$$)'
# Check for any bank account specific handling in _sync_data
ast-grep --pattern '_sync_data($$$)'
Length of output: 7688
Script:
#!/bin/bash
# Get the implementation of _sync_data method
rg -B 5 -A 15 "_sync_data\(" apps/business_central/utils.py
# Get the git history of the specific line change
git log -L 131,131:apps/business_central/utils.py
Length of output: 4978
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/business_central/exports/journal_entry/tasks.py (2)
Line range hint
49-61: Fix incomplete refactoring of balance account type logicThe credit line balance account type handling has several issues:
- The bank account type check (lines 50-52) sets an unused variable
balance_account_typebalance_account_type_credit_lineis hardcoded to 'G/L Account', making the bank account check ineffectiveApply this fix:
balance_account_type_credit_line = 'G/L Account' if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT': - balance_account_type = 'Bank Account' + balance_account_type_credit_line = 'Bank Account'
Line range hint
49-76: Consider refactoring duplicate account type logicThe logic for determining the balance account type is duplicated between credit and debit lines. Consider extracting this into a helper method for better maintainability.
Example refactor:
def _get_balance_account_type(self, workspace_id: int, account_id: str) -> str: account_attribute_type = DestinationAttribute.objects.filter( workspace_id=workspace_id, destination_id=account_id ).first() return 'Bank Account' if ( account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT' ) else 'G/L Account'Then use it like:
balance_account_type_credit_line = self._get_balance_account_type( body.workspace_id, body.accounts_payable_account_id )🧰 Tools
🪛 Ruff (0.8.0)
75-75: Local variable
balance_account_typeis assigned to but never usedRemove assignment to unused variable
balance_account_type(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/business_central/exports/journal_entry/tasks.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
apps/business_central/exports/journal_entry/tasks.py
75-75: Local variable balance_account_type is assigned to but never used
Remove assignment to unused variable balance_account_type
(F841)
🔇 Additional comments (1)
apps/business_central/exports/journal_entry/tasks.py (1)
86-86: Verify the impact of balance account type changes
The changes differentiate between credit and debit line balance account types. Please ensure this aligns with Business Central's expectations for journal entries.
✅ Verification successful
Let's examine the actual implementation and test coverage to verify the consistency of balance account type handling across the codebase.
Balance account type implementation is consistent and verified
The code correctly handles balance account types by:
- Using 'G/L Account' as the default balance account type
- Properly switching to 'Bank Account' when the account is of type 'BANK_ACCOUNT'
- Maintaining consistency in both credit and debit line implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of balance account types to ensure consistency
rg -l "balanceAccountType|BANK_ACCOUNT" --type py
# Search for test files to verify coverage
fd "test.*\.py" | rg -l "balance.*account.*type"
Length of output: 272
Script:
#!/bin/bash
# Check the implementation details of balance account type usage
rg "balanceAccountType|BANK_ACCOUNT" --type py -C 3
# Check test coverage specifically for balance account types
rg "balance.*account.*type" tests/ -C 3
Length of output: 5188
|
|
||
| account_attribute_type = DestinationAttribute.objects.filter(workspace_id=body.workspace_id, destination_id=lineitem.accounts_payable_account_id).first() | ||
| balance_account_type_debit_line = 'G/L Account' | ||
| if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT': | ||
| balance_account_type = 'Bank Account' | ||
|
|
There was a problem hiding this comment.
Fix similar refactoring issues in debit line handling
The debit line balance account type handling has the same issues as the credit line:
- The bank account type check sets an unused variable
balance_account_type balance_account_type_debit_lineis hardcoded to 'G/L Account', making the bank account check ineffective
Apply this fix:
balance_account_type_debit_line = 'G/L Account'
if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT':
- balance_account_type = 'Bank Account'
+ balance_account_type_debit_line = 'Bank Account'📝 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.
| account_attribute_type = DestinationAttribute.objects.filter(workspace_id=body.workspace_id, destination_id=lineitem.accounts_payable_account_id).first() | |
| balance_account_type_debit_line = 'G/L Account' | |
| if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT': | |
| balance_account_type = 'Bank Account' | |
| account_attribute_type = DestinationAttribute.objects.filter(workspace_id=body.workspace_id, destination_id=lineitem.accounts_payable_account_id).first() | |
| balance_account_type_debit_line = 'G/L Account' | |
| if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT': | |
| balance_account_type_debit_line = 'Bank Account' | |
🧰 Tools
🪛 Ruff (0.8.0)
75-75: Local variable balance_account_type is assigned to but never used
Remove assignment to unused variable balance_account_type
(F841)
|
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/workspaces/serializers.py (1)
218-220: Consider documenting the synchronization flow changes.The changes to charts_of_accounts handling and category import log reset appear to be part of a larger effort to simplify the synchronization logic, particularly around bank account conditions (as mentioned in the PR objectives). Consider:
- Adding a comment explaining why category import logs need to be reset when charts_of_accounts changes
- Documenting the relationship between these changes and the removal of bank account conditions
- Updating relevant documentation about the synchronization flow
Would you like me to help draft the documentation for these changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/business_central/exports/journal_entry/tasks.py(1 hunks)apps/workspaces/serializers.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/business_central/exports/journal_entry/tasks.py
🔇 Additional comments (1)
apps/workspaces/serializers.py (1)
218-220: Improved error handling, but consider additional edge cases.
The changes improve error handling by safely checking for the existence of import settings before accessing them. However, there are a few considerations:
- The code assumes only one ImportSetting per workspace (using .first()). If this is by design, consider adding a unique constraint in the model.
- Consider wrapping the category import log reset logic in a try-except block to handle potential edge cases.
Consider this enhanced implementation:
- import_setting_instance = ImportSetting.objects.filter(workspace_id=instance.id).first()
-
- if import_setting_instance and import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts:
+ try:
+ import_setting_instance = ImportSetting.objects.get(workspace_id=instance.id)
+ if import_settings.get('charts_of_accounts') != import_setting_instance.charts_of_accounts:
+ try:
+ category_import_log = ImportLog.objects.filter(
+ workspace_id=instance.id,
+ attribute_type='CATEGORY'
+ ).first()
+ if category_import_log:
+ category_import_log.last_successful_run_at = None
+ category_import_log.save()
+ except Exception as e:
+ # Log the error but don't block the update
+ logger.error(
+ f'Error resetting category import log for workspace {instance.id}: {str(e)}'
+ )
+ except ImportSetting.DoesNotExist:
+ # This is the first import setting for this workspace
+ passLet's verify the uniqueness constraint assumption:
* Fix: Adding default ccc bank account * removing bank account from import settings * bug fixed * change name * renamed * change name in JE
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #179 +/- ##
==========================================
- Coverage 96.38% 95.24% -1.14%
==========================================
Files 90 90
Lines 4981 5217 +236
==========================================
+ Hits 4801 4969 +168
- Misses 180 248 +68 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/business_central/serializers.py (1)
81-82: Consider documenting the attribute type filtering strategyThe changes across these files indicate a systematic update to bank account and attribute handling. Consider:
- Documenting the rationale for excluding these specific attribute types
- Creating a central configuration for shared attribute types
- Adding tests to verify the filtering behavior
This will help maintain consistency as the attribute system evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/business_central/exports/journal_entry/models.py(1 hunks)apps/business_central/serializers.py(1 hunks)apps/fyle/serializers.py(1 hunks)apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py(1 hunks)apps/workspaces/migrations/0006_rename_default_ccc_bank_account_id_exportsetting_default_ccc_bank_account_id_and_more.py(1 hunks)apps/workspaces/models.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/workspaces/migrations/0006_rename_default_ccc_bank_account_id_exportsetting_default_ccc_bank_account_id_and_more.py
🔇 Additional comments (5)
apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py (1)
16-17: Field names will be renamed in subsequent migration
The field names default_CCC_bank_account_id and default_CCC_bank_account_name use uppercase 'CCC' which is inconsistent with Python naming conventions. However, this is addressed in a subsequent migration that renames them to snake_case format.
Also applies to: 21-22
apps/fyle/serializers.py (1)
102-102: Verify the impact of excluding EMPLOYEE attributes
The addition of 'EMPLOYEE' to the excluded attribute types will filter out employee-related attributes from custom fields. This change aligns with the PR objective but warrants verification of its impact.
✅ Verification successful
Let me check the actual usage of EMPLOYEE attribute type in these files to understand the impact better.
Let me check one final thing to understand how the attribute_types list is used in filtering.
Adding EMPLOYEE to excluded attribute types is safe and consistent
The change is verified to be safe because:
- The
attribute_typeslist is used exclusively inFyleFieldsSerializer.format_fyle_fields()to exclude certain attribute types from custom field queries - EMPLOYEE attributes are already handled separately in the codebase through direct filtering as seen in
apps/workspaces/serializers.pyandapps/business_central/exports/helpers.py - The exclusion aligns with how other core attribute types (CATEGORY, PROJECT, etc.) are handled in the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct references to EMPLOYEE attributes that might be affected
# Check for EMPLOYEE attribute usage in code
rg -l "attribute_type.*EMPLOYEE" --type py
# Check for existing EMPLOYEE attributes in database queries or filters
ast-grep --pattern 'ExpenseAttribute.objects.filter($$$attribute_type$$$)'
Length of output: 507
Script:
#!/bin/bash
# Check the context of EMPLOYEE attribute usage in key files
rg "attribute_type.*EMPLOYEE" -B 2 -A 2 apps/mappings/tasks.py apps/business_central/exports/helpers.py apps/workspaces/serializers.py
# Check if there are any existing filters excluding attribute types
rg "attribute_types.*NOT IN" -B 2 -A 2 --type py
# Check the serializer class context
rg "class.*Serializer" -A 10 apps/fyle/serializers.py
Length of output: 3590
Script:
#!/bin/bash
# Check how attribute_types is used in filtering logic
rg -B 5 -A 5 "attribute_types = \[" apps/fyle/serializers.py
# Also check for any NOT IN conditions with these types
rg "~Q\(.*attribute_type" -B 2 -A 2 --type py
Length of output: 1287
apps/business_central/serializers.py (1)
81-82: Verify the impact of excluding BANK_ACCOUNT and COMPANY attributes
The addition of these types to the excluded list means they won't appear in custom fields. This aligns with the PR objective of modifying bank account handling.
✅ Verification successful
Based on the analysis of the shell script results, I can now provide the final response:
Excluding BANK_ACCOUNT and COMPANY attributes is safe and aligns with the codebase
The verification shows that:
- BANK_ACCOUNT is only used in specific contexts for bank account handling in journal entries and tests
- COMPANY is used alongside LOCATION and EMPLOYEE for active status checks in utils.py
- The exclusion in the serializer is used to filter out these system-managed attributes from custom fields, which is the correct behavior
The changes maintain the expected behavior where these attributes are handled as system types rather than custom fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct references to these attributes that might be affected
# Check for attribute usage in code
rg -l "attribute_type.*(BANK_ACCOUNT|COMPANY)" --type py
# Check for existing attributes in database queries or filters
ast-grep --pattern 'DestinationAttribute.objects.filter($$$attribute_type$$$)'
Length of output: 265
Script:
#!/bin/bash
# Let's check the actual usage context of these attribute types
# Check the content of files that reference these attributes
rg "attribute_type.*(BANK_ACCOUNT|COMPANY)" -B 2 -A 2 --type py
# Check for any DestinationAttribute queries with different patterns
rg "DestinationAttribute.*filter" -B 2 -A 2 --type py
Length of output: 28544
apps/business_central/exports/journal_entry/models.py (1)
47-49: LGTM! Bank account selection logic looks correct.
The conditional logic appropriately selects the bank account ID based on the expense's fund source, using the newly added CCC-specific bank account for corporate credit card expenses.
Let's verify that both bank account fields are properly set in the export settings:
✅ Verification successful
Bank account selection logic is properly implemented and tested
Based on the verification results:
- Both bank account fields (
default_bank_account_idanddefault_ccc_bank_account_id) are properly defined in the Workspace model - The CCC fund source logic is being used in multiple places:
- Journal Entry creation (
business_central/exports/journal_entry/models.py) - Task processing (
workspaces/tasks.py) - Test coverage exists in multiple test files
- Journal Entry creation (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both bank account fields are being set in the codebase
rg -A 2 "default_bank_account_id.*=|default_ccc_bank_account_id.*=" --type py
Length of output: 1827
Script:
#!/bin/bash
# Let's verify if these fields are being properly set during creation/updates
ast-grep --pattern 'default_ccc_bank_account_id = $_'
Length of output: 9015
Script:
#!/bin/bash
# Let's check if there are any tests validating this CCC bank account logic
rg -A 5 "fund_source.*=.*'CCC'" --type py
Length of output: 3395
apps/workspaces/models.py (1)
189-190: LGTM! New CCC bank account fields are well-structured.
The new fields follow the existing pattern and are properly defined with appropriate field types and help text.
Let's verify the migration files for these new fields:
✅ Verification successful
Migration files show proper field addition and case correction
The fields were properly added in migration 0005 with correct field types and help text, followed by a case correction in migration 0006 (changing from default_CCC_bank_account to default_ccc_bank_account). The current implementation in the model matches the latest migration state.
- Migration 0005: Added the fields with StringNullField type and appropriate help text
- Migration 0006: Renamed the fields to follow consistent lowercase naming convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check migration files for the new fields
fd "^[0-9]{4}.+\.py$" --type f --exec rg -l "default_ccc_bank_account"
Length of output: 193
Script:
#!/bin/bash
# Check the content of the migration file to verify the field changes
cat ./apps/workspaces/migrations/0006_rename_default_ccc_bank_account_id_exportsetting_default_ccc_bank_account_id_and_more.py
Length of output: 766
Script:
#!/bin/bash
# Check the previous migration to see the initial field addition
cat ./apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py
Length of output: 906
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/li/901604887942
Summary by CodeRabbit