Fix: Adding default ccc bank account #180
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/workspaces/models.py (1)
189-190: LGTM! Model changes are consistent with existing patterns.The new CCC bank account fields are well-placed and follow the established patterns in the codebase. Consider adding documentation about when these fields should be configured.
Consider adding a docstring to the ExportSetting class explaining the purpose of CCC-specific fields:
class ExportSetting(BaseModel): """ Table to store export settings + + CCC (Corporate Credit Card) specific fields: + - default_CCC_bank_account_name: Name of the bank account for CCC transactions + - default_CCC_bank_account_id: ID of the bank account for CCC transactions """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/business_central/exports/journal_entry/models.py(1 hunks)apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py(1 hunks)apps/workspaces/models.py(1 hunks)
🔇 Additional comments (2)
apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py (1)
13-24: LGTM! Migration is well-structured and backward compatible.
The migration correctly adds the new CCC bank account fields with appropriate field types and null constraints, ensuring backward compatibility with existing records.
apps/business_central/exports/journal_entry/models.py (1)
47-49: Verify CCC bank account ID availability.
While the logic is correct, we should ensure that default_CCC_bank_account_id is available when fund_source is 'CCC'.
Let's check for any existing CCC expenses that might be affected:
| if expense.fund_source == 'CCC': | ||
| accounts_payable_account_id = export_settings.default_CCC_bank_account_id | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing CCC bank account.
Consider adding validation to handle cases where default_CCC_bank_account_id is None for CCC expenses.
if expense.fund_source == 'CCC':
+ if not export_settings.default_CCC_bank_account_id:
+ raise ValueError('CCC bank account not configured for workspace')
accounts_payable_account_id = export_settings.default_CCC_bank_account_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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if expense.fund_source == 'CCC': | |
| accounts_payable_account_id = export_settings.default_CCC_bank_account_id | |
| if expense.fund_source == 'CCC': | |
| if not export_settings.default_CCC_bank_account_id: | |
| raise ValueError('CCC bank account not configured for workspace') | |
| accounts_payable_account_id = export_settings.default_CCC_bank_account_id |
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
apps/workspaces/models.py
Outdated
| @@ -186,6 +186,8 @@ class ExportSetting(BaseModel): | |||
| ) | |||
| default_bank_account_name = StringNullField(help_text='Bank account name') | |||
| default_bank_account_id = StringNullField(help_text='Bank Account ID') | |||
| default_CCC_bank_account_name = StringNullField(help_text='CCC Bank account name') | |||
There was a problem hiding this comment.
default_ccc_bank_account_name
apps/workspaces/models.py
Outdated
| @@ -186,6 +186,8 @@ class ExportSetting(BaseModel): | |||
| ) | |||
| default_bank_account_name = StringNullField(help_text='Bank account name') | |||
| default_bank_account_id = StringNullField(help_text='Bank Account ID') | |||
| default_CCC_bank_account_name = StringNullField(help_text='CCC Bank account name') | |||
| default_CCC_bank_account_id = StringNullField(help_text='CCC Bank Account ID') | |||
There was a problem hiding this comment.
default_ccc_bank_account_id
| accounts_payable_account_id = export_settings.default_bank_account_id | ||
|
|
||
| if expense.fund_source == 'CCC': | ||
| accounts_payable_account_id = export_settings.default_CCC_bank_account_id |
There was a problem hiding this comment.
default_ccc_bank_account_id
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/business_central/serializers.py (1)
81-82: LGTM! Consider adding documentation about attribute filtering.The addition of "BANK_ACCOUNT" to the excluded attribute types is correct and aligns with the CCC bank account integration.
Consider adding a docstring comment explaining that this method returns non-standard fields by excluding common attribute types:
def format_business_central_fields(self, workspace_id): + """ + Returns a list of non-standard Business Central fields by excluding common attribute types + like VENDOR, ACCOUNT, BANK_ACCOUNT etc. + + Args: + workspace_id: The ID of the workspace + + Returns: + List of unique attribute types and their display names + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/business_central/serializers.py(1 hunks)
🔇 Additional comments (1)
apps/business_central/serializers.py (1)
81-82: Verify integration with CCC bank account feature.
Let's verify that this change properly integrates with the new CCC bank account functionality mentioned in the PR.
✅ Verification successful
Integration with CCC bank account feature is properly implemented
The code changes are properly integrated with the CCC bank account functionality. The evidence shows:
- CCC bank account fields are properly defined in the workspace models
- Bank account synchronization is implemented and tested
- Journal entry exports correctly handle CCC bank accounts
- The serializer's BANK_ACCOUNT attribute type is consistent with the rest of the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related CCC bank account handling code
echo "Checking for CCC bank account related code..."
# Look for CCC bank account related models
echo "Looking for model definitions..."
rg -l "default_CCC_bank_account"
# Look for bank account handling in business logic
echo "Looking for bank account handling..."
rg -l "BANK_ACCOUNT.*attribute_type"
# Look for related tests
echo "Looking for related test coverage..."
rg -l "test.*bank.*account" --type=python
Length of output: 654
Script:
#!/bin/bash
# Let's check the actual implementation and usage of bank account related code
# Check the models file for bank account implementation
echo "Checking bank account implementation in models..."
rg -A 10 "BANK_ACCOUNT" apps/business_central/serializers.py
# Look for bank account related tests with proper file type
echo "Looking for test files..."
fd "test.*\.py$" | xargs rg -l "bank.*account"
# Check the export settings and journal entry models
echo "Checking export settings and journal entries..."
cat apps/workspaces/models.py | grep -A 10 "default_ccc_bank_account"
cat apps/business_central/exports/journal_entry/models.py | grep -A 10 "BANK_ACCOUNT"
Length of output: 946
Script:
#!/bin/bash
# Let's examine the test files and related implementations more closely
# Check the test files content
echo "Checking test file contents..."
cat tests/test_business_central/test_utils.py | grep -A 10 -B 10 "bank.*account"
cat tests/test_workspaces/test_view.py | grep -A 10 -B 10 "bank.*account"
# Look for CCC specific bank account configuration
echo "Checking CCC specific configurations..."
rg -A 5 "CCC.*bank.*account"
# Check for bank account related constants or enums
echo "Checking for bank account related constants..."
rg -A 5 "BANK_ACCOUNT" apps/business_central/
Length of output: 7260
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
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
|
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
added
deploy
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspaces/migrations/0005_exportsetting_default_ccc_bank_account_id_and_more.py
🔇 Additional comments (1)
apps/workspaces/migrations/0006_rename_default_ccc_bank_account_id_exportsetting_default_ccc_bank_account_id_and_more.py (1)
1-23: LGTM! Migration follows Django naming conventions.
The migration correctly renames the fields from CamelCase to snake_case, following Python/Django naming conventions (PEP 8).
| migrations.RenameField( | ||
| model_name='exportsetting', | ||
| old_name='default_CCC_bank_account_id', | ||
| new_name='default_ccc_bank_account_id', | ||
| ), | ||
| migrations.RenameField( | ||
| model_name='exportsetting', | ||
| old_name='default_CCC_bank_account_name', | ||
| new_name='default_ccc_bank_account_name', | ||
| ), |
There was a problem hiding this comment.
💡 Codebase verification
Update required: Found reference to old field name in business_central export model
The field default_CCC_bank_account_id in apps/business_central/exports/journal_entry/models.py still uses the old CamelCase naming convention and needs to be updated to match the new snake_case field name from the migration.
- Update
export_settings.default_CCC_bank_account_idtoexport_settings.default_ccc_bank_account_idinapps/business_central/exports/journal_entry/models.py
🔗 Analysis chain
Verify field name updates across the codebase.
Let's ensure all references to these fields have been updated to use the new snake_case names.
Let's check the content of the file that still contains references to the old field name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old CamelCase field names
# which might cause runtime errors if not updated.
echo "Checking for old field name references..."
rg -l "default_CCC_bank_account" \
--type py \
--glob '!**/migrations/*.py'
# Check for any Django model or serializer references
ast-grep --pattern 'class $_(models.Model):
$$$
default_CCC_bank_account_$_
$$$'
ast-grep --pattern 'class $_(serializers.ModelSerializer):
$$$
default_CCC_bank_account_$_
$$$'
Length of output: 411
Script:
#!/bin/bash
# Check the content of the file to see the exact usage
rg "default_CCC_bank_account" apps/business_central/exports/journal_entry/models.py -A 2 -B 2
Length of output: 321
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
|
|
* Fix: Removing condition for bank account in sync data * fix line item balanceAccountType check * bug fix * bug fix * Fix: Adding default ccc bank account (#180) * Fix: Adding default ccc bank account * removing bank account from import settings * bug fixed * change name * renamed * change name in JE
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
New Features
Bug Fixes
Migrations