Feat: Add chart of accounts type when importing accounts to category in Fyle #177
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe pull request introduces several modifications across multiple files to enhance the filtering capabilities and import processes related to charts of accounts. Key changes include the addition of a new Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
apps/workspaces/migrations/0004_importsetting_charts_of_accounts.py (1)
18-18: Consider adding a size limit to the ArrayFieldThe ArrayField is configured with
size=None, which means there's no limit on the number of elements. Consider adding a reasonable size limit to prevent potential memory issues with very large arrays.- field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), default=apps.workspaces.models.get_default_chart_of_accounts, help_text='list of chart of account types to be imported into Fyle', size=None), + field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), default=apps.workspaces.models.get_default_chart_of_accounts, help_text='list of chart of account types to be imported into Fyle', size=10),apps/mappings/imports/tasks.py (1)
Line range hint
17-24: Update docstring to include the new parameter.The docstring should be updated to include documentation for the
charts_of_accountsparameter and its purpose.def trigger_import_via_schedule(workspace_id: int, destination_field: str, source_field: str, charts_of_accounts: List[str] = None, is_custom: bool = False): """ Trigger import via schedule :param workspace_id: Workspace id :param destination_field: Destination field :param source_field: Type of attribute (e.g. 'CATEGORY', 'MERCHANT', 'COST_CENTER', 'PROJECT') + :param charts_of_accounts: List of chart of account types to filter by + :param is_custom: Flag indicating if this is a custom field import """apps/mappings/imports/queues.py (2)
51-52: Document the purpose of new parametersThe empty list and boolean parameters added to the custom fields mapping task call need documentation explaining their purpose and expected values.
Consider adding a comment explaining what these parameters represent:
chain.append( 'apps.mappings.imports.tasks.trigger_import_via_schedule', workspace_id, custom_fields_mapping_setting.destination_field, custom_fields_mapping_setting.source_field, + # TODO: Document the purpose of these parameters [], True )
Line range hint
1-57: Consider adding type hints and error recoveryThe chain pattern implementation could benefit from:
- Type hints for better code maintainability and IDE support
- Error recovery mechanism to handle failures in individual chain steps without affecting the entire import process
Example type hints:
from typing import List from django_q.tasks import Chain from fyle_accounting_mappings.models import MappingSetting def chain_import_fields_to_fyle(workspace_id: int) -> None: mapping_settings: List[MappingSetting] = MappingSetting.objects.filter(...)apps/mappings/imports/modules/categories.py (1)
Line range hint
34-77: Consider implementing chart of accounts filteringThe
construct_fyle_payloadmethod processes all categories without considering the newcharts_of_accountsfilter. Consider updating the logic to filter categories based on their associated chart of accounts.You might want to:
- Update the destination attributes query to include chart of accounts filtering
- Add validation in the payload construction to ensure categories belong to the specified charts of accounts
- Update the mapping creation logic to respect the chart of accounts filter
Would you like assistance in implementing these changes?
apps/workspaces/models.py (1)
154-155: Add docstring and consider function placement.The function would benefit from:
- A docstring explaining its purpose and return value
- Consider moving it as a class method within
ImportSettingsince it's only used there-def get_default_chart_of_accounts(): - return ['Expense'] +class ImportSetting(BaseModel): + @staticmethod + def get_default_chart_of_accounts(): + """ + Returns the default list of chart of account types to be imported into Fyle. + + Returns: + list: A list containing default chart of account types + """ + return ['Expense']apps/workspaces/serializers.py (1)
218-224: Consider adding error handling for the ImportLog updateThe logic for resetting the import timestamp when charts_of_accounts changes is correct. However, consider adding error handling for the save operation.
if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts: 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() + try: + category_import_log.last_successful_run_at = None + category_import_log.save() + except Exception as e: + logger.error( + 'Error while resetting category import log - Workspace ID: %s, Error: %s', + instance.id, + str(e) + )apps/mappings/imports/modules/base.py (3)
2-2: Remove unused importThe
Qimport from django.db.models is not used in the code. Thecopyimport should be kept as it's used in theconstruct_attributes_filtermethod.import copy from datetime import datetime, timedelta, timezone from typing import List -from django.db.models import QAlso applies to: 6-6
🧰 Tools
🪛 Ruff (0.7.0)
2-2:
copyimported but unusedRemove unused import:
copy(F401)
73-77: LGTM! Consider adding type hintsThe implementation of charts_of_accounts filtering is well-structured. The code correctly:
- Creates a copy to avoid modifying the original filters
- Checks for attribute type to prevent circular filtering
- Validates charts_of_accounts existence and non-emptiness
Consider adding type hints for better code maintainability:
- def construct_attributes_filter(self, attribute_type: str, paginated_destination_attribute_values: List[str] = []): + def construct_attributes_filter( + self, + attribute_type: str, + paginated_destination_attribute_values: List[str] = [] + ) -> dict[str, Any]:
Line range hint
234-235: Consider making batch size configurableThe batch size of 200 is hardcoded in multiple places. Consider making this a class constant or configuration value for better maintainability and flexibility.
class Base: + BATCH_SIZE = 200 """ The Base class for all the modules """Then update the references:
- import_log.total_batches_count = math.ceil(destination_attributes_count / 200) + import_log.total_batches_count = math.ceil(destination_attributes_count / self.BATCH_SIZE)🧰 Tools
🪛 Ruff (0.7.0)
2-2:
copyimported but unusedRemove unused import:
copy(F401)
6-6:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/mappings/imports/modules/base.py(3 hunks)apps/mappings/imports/modules/categories.py(2 hunks)apps/mappings/imports/queues.py(2 hunks)apps/mappings/imports/tasks.py(3 hunks)apps/workspaces/migrations/0004_importsetting_charts_of_accounts.py(1 hunks)apps/workspaces/models.py(2 hunks)apps/workspaces/serializers.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
apps/mappings/imports/modules/base.py
2-2: copy imported but unused
Remove unused import: copy
(F401)
6-6: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
217-217: Local variable destination_attributes is assigned to but never used
Remove assignment to unused variable destination_attributes
(F841)
🔇 Additional comments (13)
apps/workspaces/migrations/0004_importsetting_charts_of_accounts.py (2)
10-12: Verify migration dependency exists
Let's confirm that the dependent migration '0003_remove_advancedsetting_schedule_id_and_more' exists.
✅ Verification successful
Migration dependency '0003_remove_advancedsetting_schedule_id_and_more' exists
The dependent migration file 0003_remove_advancedsetting_schedule_id_and_more.py is present in the apps/workspaces/migrations/ directory, confirming that the migration dependency is correctly specified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the dependent migration exists
# Expected: Should find the 0003 migration file
fd "0003.*\.py" apps/workspaces/migrations/
Length of output: 124
18-18: Verify the default value function implementation
Let's verify that the get_default_chart_of_accounts function exists and returns the expected type (list of strings).
✅ Verification successful
Function implementation verified and returns correct type
The get_default_chart_of_accounts function exists in apps/workspaces/models.py and returns a list containing 'Expense', which is the correct type (list of strings) for the ArrayField with CharField base field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the default value function exists and its implementation
# Expected: Function should exist and return a list containing 'Expense'
# Search for the function definition
ast-grep --pattern 'def get_default_chart_of_accounts():
$$$
'
# Also check for any direct usage or references
rg "get_default_chart_of_accounts" -A 3
Length of output: 1214
apps/mappings/imports/tasks.py (4)
Line range hint 1-15: LGTM! Import statements and constants are well-organized.
The addition of List from typing module properly supports the new type hints, and the constant mapping remains clear and maintainable.
27-31: LGTM! Clean refactoring of argument handling.
The introduction of the args dictionary improves code maintainability and makes parameter passing more explicit.
39-44: LGTM! Object instantiation is properly handled.
The use of kwargs unpacking with the args dictionary maintains clean code while supporting both custom and standard field imports.
36-38: Verify handling of existing category imports.
The new charts_of_accounts parameter might affect existing category imports. Let's verify that existing imports continue to work as expected with the default None value.
✅ Verification successful
No compatibility issues with existing category imports
Based on the code analysis, the addition of charts_of_accounts parameter is safely handled:
- The parameter is optional with a default value of
Nonein thetrigger_import_via_schedulefunction signature. - It's only passed to the Category class constructor when
source_field == 'CATEGORY'. - The Category class properly accepts and stores this parameter without any breaking changes to the core import functionality.
- Existing test cases in the codebase continue to work with this parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing category import calls to ensure compatibility
rg -l "trigger_import_via_schedule.*CATEGORY" | while read -r file; do
echo "=== $file ==="
rg -A 2 "trigger_import_via_schedule.*CATEGORY" "$file"
done
Length of output: 62
Script:
#!/bin/bash
# Let's first check for any CATEGORY related imports in the codebase
rg -B 2 -A 2 "source_field.*CATEGORY"
# Also check for any test cases related to category imports
rg -B 2 -A 2 "test.*category.*import" -i
Length of output: 5111
Script:
#!/bin/bash
# Let's check the implementation of the import function to understand the default behavior
ast-grep --pattern 'def trigger_import_via_schedule($$$)'
# Also check the base implementation for category imports
ast-grep --pattern 'class Categories($$$)'
Length of output: 104
Script:
#!/bin/bash
# Let's look at the full implementation of the tasks.py file
cat apps/mappings/imports/tasks.py
# And check the categories module implementation
cat apps/mappings/imports/modules/categories.py
Length of output: 4781
apps/mappings/imports/queues.py (1)
24-25: Verify error handling for charts_of_accounts parameter
The addition of charts_of_accounts parameter looks good, but we should ensure proper error handling in the downstream task.
✅ Verification successful
Error handling for charts_of_accounts parameter is properly implemented
The codebase shows proper handling of the charts_of_accounts parameter:
- Type validation through type hints (
List[str]) - Optional parameter with default value of
None - Proper filtering implementation in
base.pythat safely checks for attribute existence and list length - Clean integration in the Categories module constructor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how the parameter is handled in the task
ast-grep --pattern 'def trigger_import_via_schedule($$$charts_of_accounts$$$):
$$$
try {
$$$
} except $$$'
Length of output: 118
Script:
#!/bin/bash
# Let's try a broader search for the import task implementation
rg -A 10 "trigger_import_via_schedule"
# Also search for any error handling related to charts_of_accounts
rg -A 5 "charts_of_accounts.*try|try.*charts_of_accounts"
# Search for the class/function that processes CATEGORY imports
ast-grep --pattern 'class $_ {
$$$
CATEGORY$$$
$$$
}'
Length of output: 3428
Script:
#!/bin/bash
# Let's check the implementation details of the import process
rg -A 15 "def import_" apps/mappings/imports/
# Look for any validation or error handling for charts_of_accounts
rg -A 5 "charts_of_accounts" apps/mappings/imports/
# Check for any type validation or parameter processing
ast-grep --pattern 'charts_of_accounts: List[str]'
Length of output: 6691
apps/mappings/imports/modules/categories.py (1)
14-17: 🛠️ Refactor suggestion
Add documentation and consider parameter validation
The new charts_of_accounts parameter lacks documentation and validation. Additionally, it's stored but not used in any visible class methods.
Consider applying these improvements:
def __init__(self, workspace_id: int, destination_field: str, sync_after: datetime, charts_of_accounts: List[str]):
+ """
+ Initialize Category module
+ :param workspace_id: Workspace ID
+ :param destination_field: Destination field
+ :param sync_after: Sync after datetime
+ :param charts_of_accounts: List of chart of accounts to filter by
+ """
+ if not isinstance(charts_of_accounts, list):
+ raise TypeError("charts_of_accounts must be a list of strings")
+
self.charts_of_accounts = charts_of_accountsLet's verify how this parameter is used:
✅ Verification successful
Let me gather more context about the base class implementation to understand how this parameter is used.
Let me get one final piece of context to understand the full implementation.
The review comment's concerns about documentation and validation are valid, but the parameter usage is properly implemented
The charts_of_accounts parameter is actually used in the base class's construct_attributes_filter method, where it filters account details based on the provided chart of accounts when attribute_type is not 'CATEGORY'. The implementation follows a proper inheritance pattern where the Categories class extends this filtering capability.
While the parameter is used correctly, the documentation suggestions remain valid:
def __init__(self, workspace_id: int, destination_field: str, sync_after: datetime, charts_of_accounts: List[str]):
+ """
+ Initialize Category module
+ :param workspace_id: Workspace ID
+ :param destination_field: Destination field
+ :param sync_after: Sync after datetime
+ :param charts_of_accounts: List of chart of accounts to filter account details
+ """
self.charts_of_accounts = charts_of_accounts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of charts_of_accounts in the codebase
rg "charts_of_accounts" --type py
Length of output: 1530
Script:
#!/bin/bash
# Get the Base class implementation to understand the parameter usage
ast-grep --pattern 'class Base {
$$$
}'
# Also get the specific method where charts_of_accounts is used
rg -B 2 -A 4 "account_filters\[\"detail__category__in\"\] = self.charts_of_accounts" --type py
Length of output: 679
Script:
#!/bin/bash
# Get the full implementation of base.py to understand the complete context
cat apps/mappings/imports/modules/base.py
Length of output: 14505
apps/workspaces/models.py (2)
235-235: Consider adding value validation for chart of account types.
The field looks good, but consider adding validation to ensure only valid chart of account types are stored. This could be done through:
- Model validation
- Choice field for valid types
Let's check if there are any existing chart of account type constants or validations in the codebase:
235-235: Verify migration handling for existing records.
The new field has a default value function, but we should verify:
- The migration file exists and is properly configured
- The migration handles existing records appropriately
Let's check the migration file:
✅ Verification successful
Let me check the content of the migration file to verify how it handles the field addition.
Migration is properly configured with default value function
The migration file correctly:
- Adds the new
charts_of_accountsfield toImportSettingmodel - Uses the same default value function
get_default_chart_of_accounts - Configures the field as an ArrayField with CharField(max_length=100)
- Includes proper help text
Since the field is added with a default value function, existing records will automatically get the default value when the migration runs, ensuring data consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for related migration file
fd "0004_importsetting_charts_of_accounts.py" -t f
# Check if there are any existing records that might be affected
ast-grep --pattern 'class Migration(migrations.Migration):
$$$
operations = [
$$$
AddField(
$$$
name="charts_of_accounts",
$$$
),
$$$
]'
Length of output: 297
Script:
#!/bin/bash
# Check the content of the migration file
cat apps/workspaces/migrations/0004_importsetting_charts_of_accounts.py
Length of output: 769
apps/workspaces/serializers.py (3)
16-16: LGTM: Import statement is correctly placed
The ImportLog model import is properly organized with other model imports and is required for the new functionality.
230-231: LGTM: Safe update of charts_of_accounts field
The field is correctly added to the defaults dictionary within a transaction block, using safe dictionary access.
186-186: Verify the model field definition for charts_of_accounts
The field is added to the serializer, but we should verify its model definition to ensure proper validation and serialization.
✅ Verification successful
Model field definition for charts_of_accounts is properly configured
The field is correctly defined in the model as an ArrayField with CharField base field, default value function, and appropriate help text. This matches the serializer's implicit field inheritance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the ImportSetting model definition for charts_of_accounts field
rg -A 5 "class ImportSetting.*:|charts_of_accounts" apps/workspaces/models.py
Length of output: 624
apps/mappings/imports/queues.py
Outdated
| print() | ||
|
|
There was a problem hiding this comment.
Remove empty print statement
This empty print statement appears to be a debugging artifact and should be removed as it adds unnecessary noise to the logs.
- print()📝 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.
| print() |
| @@ -24,6 +27,7 @@ def trigger_import(self): | |||
| """ | |||
| Trigger import for Category module | |||
| """ | |||
| print('herer') | |||
There was a problem hiding this comment.
Remove debug print statement
Debug print statements should not be committed to production code.
- print('herer')📝 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.
| print('herer') |
| filters['value__in'] = paginated_destination_attribute_values | ||
| filters["value__in"] = paginated_destination_attribute_values | ||
|
|
||
| account_filters = filters.copy() |
There was a problem hiding this comment.
why are we doing this copy thing?
There was a problem hiding this comment.
we are doing this QBO as well, thought must be for some reason, like mutability
| @@ -24,6 +27,7 @@ def trigger_import(self): | |||
| """ | |||
| Trigger import for Category module | |||
| """ | |||
| print('herer') | |||
apps/mappings/imports/queues.py
Outdated
| @@ -14,12 +14,15 @@ def chain_import_fields_to_fyle(workspace_id): | |||
| import_settings = ImportSetting.objects.get(workspace_id=workspace_id) | |||
| chain = Chain() | |||
|
|
|||
| print() | |||
apps/mappings/imports/tasks.py
Outdated
| 'destination_field': destination_field, | ||
| 'sync_after': sync_after, | ||
| } | ||
|
|
||
| if is_custom: |
There was a problem hiding this comment.
two is_custom coditions are here
| if source_field == 'CATEGORY': | ||
| args['charts_of_accounts'] = charts_of_accounts | ||
|
|
||
| if is_custom: |
| @@ -229,6 +232,7 @@ class ImportSetting(BaseModel): | |||
| import_categories = BooleanFalseField(help_text='toggle for import of chart of accounts from Business Central') | |||
| import_vendors_as_merchants = BooleanFalseField(help_text='toggle for import of vendors as merchant from Business Central') | |||
| workspace = models.OneToOneField(Workspace, on_delete=models.PROTECT, help_text='Reference to Workspace model', related_name="import_settings") | |||
| charts_of_accounts = ArrayField(base_field=models.CharField(max_length=100), default=get_default_chart_of_accounts, help_text='list of chart of account types to be imported into Fyle') | |||
There was a problem hiding this comment.
rename to import_charts_of_accounts, just to maintain consistency
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/business_central/utils.py (1)
120-124: Add documentation and improve error handlingWhile the category handling logic is functional, consider these improvements:
- Add a comment explaining why 'x0020' maps to 'Others'
- Add validation for null/undefined category values
if attribute_type == 'ACCOUNT' and 'category' in detail: + # '_x0020_' is Excel's encoding for space, mapped to 'Others' for better readability + if detail['category'] is None: + detail['category'] = 'Others' if detail['category'] == '_x0020_': detail['category'] = 'Others' else: detail['category'] = unescape(detail['category'])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/business_central/utils.py(4 hunks)apps/mappings/imports/modules/categories.py(1 hunks)apps/mappings/imports/queues.py(2 hunks)requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mappings/imports/modules/categories.py
- apps/mappings/imports/queues.py
🔇 Additional comments (3)
apps/business_central/utils.py (3)
5-5: LGTM: Import statement is correctly placed
The addition of unescape from openpyxl.utils.escape is appropriate for handling Excel-encoded category values.
376-376: LGTM: Added trailing newline
Following PEP 8 style guide recommendation to end the file with a newline.
120-124: Verify error handling for category processing
While the category processing logic is implemented, we should verify error handling for API failures during category processing.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have enough information about error handling in the codebase.
Category processing has robust error handling
The code has comprehensive error handling for category processing:
- Generic error handling through
handle_business_central_exceptionsdecorator that catches and logs all exceptions - Specific error handling for category mapping in
__validate_category_mapping:- Creates detailed error records with
Errorobjects - Tracks error repetition counts
- Logs errors with proper context
- Creates detailed error records with
- Dimension-related errors are tracked in
dimension_error_logfield - Failed category processing is properly reflected in accounting export status and error details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling patterns in category processing
# Look for try-catch blocks or error handling around category processing
rg -B 5 -A 5 "try:|except:|raise|error|Exception" apps/business_central/
# Look for logging statements around category processing
rg -B 5 -A 5 "logger\." apps/business_central/
Length of output: 63652
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/mappings/imports/modules/base.py (1)
6-6: Remove unused importThe
Qobject fromdjango.db.modelsis imported but never used in the code.-from django.db.models import Q🧰 Tools
🪛 Ruff (0.7.0)
6-6:
django.db.models.Qimported but unusedRemove unused import:
django.db.models.Q(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/business_central/utils.py(4 hunks)apps/mappings/imports/modules/base.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/business_central/utils.py
🧰 Additional context used
🪛 Ruff (0.7.0)
apps/mappings/imports/modules/base.py
6-6: django.db.models.Q imported but unused
Remove unused import: django.db.models.Q
(F401)
217-217: Local variable destination_attributes is assigned to but never used
Remove assignment to unused variable destination_attributes
(F841)
🔇 Additional comments (1)
apps/mappings/imports/modules/base.py (1)
73-77:
Verify charts_of_accounts initialization
The code checks for self.charts_of_accounts using hasattr, but this attribute is not initialized in the class's __init__ method. This could lead to inconsistent behavior.
Let's verify where this attribute is set:
Consider adding the attribute to the constructor:
def __init__(
self,
workspace_id: int,
source_field: str,
destination_field: str,
platform_class_name: str,
sync_after:datetime,
+ charts_of_accounts: List[str] = None,
):
self.workspace_id = workspace_id
self.source_field = source_field
self.destination_field = destination_field
self.platform_class_name = platform_class_name
self.sync_after = sync_after
+ self.charts_of_accounts = charts_of_accounts or []| @@ -205,7 +214,7 @@ def construct_payload_and_import_to_fyle( | |||
| is_auto_sync_status_allowed = self.get_auto_sync_permission() | |||
|
|
|||
| filters = self.construct_attributes_filter(self.destination_field) | |||
|
|
|||
| destination_attributes = DestinationAttribute.objects.filter(**filters) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant database query
The destination_attributes variable is assigned but never used, creating an unnecessary database query. The same filter is applied later for the count and in get_destination_attributes_generator.
- destination_attributes = DestinationAttribute.objects.filter(**filters)
destination_attributes_count = DestinationAttribute.objects.filter(**filters).count()📝 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.
| destination_attributes = DestinationAttribute.objects.filter(**filters) |
🧰 Tools
🪛 Ruff (0.7.0)
217-217: Local variable destination_attributes is assigned to but never used
Remove assignment to unused variable destination_attributes
(F841)
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/workspaces/serializers.py (1)
186-186: Add field type and validation for charts_of_accountsConsider explicitly defining the field with its type and validation rules to ensure data integrity.
class ImportSettingFilterSerializer(serializers.ModelSerializer): """ Import Settings Filtered serializer """ + charts_of_accounts = serializers.ListField( + child=serializers.CharField(), + required=False, + allow_empty=True + ) class Meta: model = ImportSetting fields = [ 'import_categories', 'import_vendors_as_merchants', 'charts_of_accounts' ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/business_central/utils.py(3 hunks)apps/mappings/imports/modules/base.py(2 hunks)apps/mappings/imports/modules/categories.py(1 hunks)apps/mappings/imports/tasks.py(3 hunks)apps/workspaces/models.py(2 hunks)apps/workspaces/serializers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/business_central/utils.py
- apps/mappings/imports/modules/base.py
- apps/mappings/imports/modules/categories.py
- apps/mappings/imports/tasks.py
- apps/workspaces/models.py
🔇 Additional comments (1)
apps/workspaces/serializers.py (1)
230-231: Add validation for charts_of_accounts values
Consider adding validation to ensure that only valid chart of account types are accepted.
Let's verify the valid chart of account types:
| if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts: | ||
| 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() | ||
|
|
There was a problem hiding this comment.
Add null checks and optimize database operations
The current implementation has potential issues:
- Missing null checks before accessing
instance.import_settings.charts_of_accounts - The ImportLog update should be within the transaction block
Consider this safer implementation:
- if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts:
- 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()
+ with transaction.atomic():
+ old_charts_of_accounts = getattr(instance.import_settings, 'charts_of_accounts', None)
+ new_charts_of_accounts = import_settings.get('charts_of_accounts')
+
+ if old_charts_of_accounts != new_charts_of_accounts:
+ ImportLog.objects.filter(
+ workspace_id=instance.id,
+ attribute_type='CATEGORY'
+ ).update(last_successful_run_at=None)📝 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 import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts: | |
| 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() | |
| with transaction.atomic(): | |
| old_charts_of_accounts = getattr(instance.import_settings, 'charts_of_accounts', None) | |
| new_charts_of_accounts = import_settings.get('charts_of_accounts') | |
| if old_charts_of_accounts != new_charts_of_accounts: | |
| ImportLog.objects.filter( | |
| workspace_id=instance.id, | |
| attribute_type='CATEGORY' | |
| ).update(last_successful_run_at=None) |
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/test_mappings/test_imports/test_modules/test_categories.py (1)
Line range hint
1-244: Consider restructuring tests for better coverage of charts_of_accounts featureThe current test structure could be improved to better validate the new charts_of_accounts functionality:
- Consider creating a dedicated test class or module for charts_of_accounts scenarios
- Update test fixtures in
destination_attributes_datato include charts_of_accounts test cases- Add integration tests to verify end-to-end behavior with the platform connector
- Consider adding parameterized tests to cover various charts_of_accounts configurations
tests/test_mappings/test_imports/test_modules/test_base.py (3)
70-70: Add test cases for different charts_of_accounts scenariosThe test should cover various scenarios:
- Empty charts_of_accounts (current case)
- Non-empty charts_of_accounts
- Invalid charts_of_accounts data
This will ensure robust testing of the sync behavior under different conditions.
540-540: Add test cases for error resolution with charts_of_accountsThe test should verify error resolution behavior when:
- Charts of accounts contain invalid entries
- Charts of accounts affect category mapping resolution
- Charts of accounts data is malformed
This will ensure the error handling is robust across different scenarios.
Line range hint
40-540: Consider adding dedicated test module for charts_of_accounts functionalityWhile the current changes maintain test coverage for existing functionality, consider creating a dedicated test module or test class that comprehensively covers the charts_of_accounts feature. This would include:
- Integration tests with different charts_of_accounts configurations
- Edge cases and error scenarios
- Performance tests with large charts_of_accounts lists
- Boundary testing for charts_of_accounts data
This will ensure the new feature is thoroughly tested in isolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/test_fyle/fixtures.py(5 hunks)tests/test_mappings/test_imports/test_modules/test_base.py(3 hunks)tests/test_mappings/test_imports/test_modules/test_categories.py(5 hunks)tests/test_workspaces/test_view.py(2 hunks)
🔇 Additional comments (7)
tests/test_mappings/test_imports/test_modules/test_categories.py (3)
194-194: 🛠️ Refactor suggestion
Verify Fyle payload construction with charts_of_accounts
The test should verify that the constructed Fyle payload correctly incorporates the charts_of_accounts filter. Consider:
- Adding test cases with non-empty charts_of_accounts
- Updating test fixtures to include charts_of_accounts scenarios
- Verifying payload structure matches Fyle's expectations when filtered by charts_of_accounts
124-124: 🛠️ Refactor suggestion
Verify auto-creation behavior with charts_of_accounts filter
The test should verify that the auto-creation logic correctly handles the charts_of_accounts parameter. Consider adding test cases to verify:
- Auto-creation of attributes filtered by charts_of_accounts
- Handling of existing mappings with charts_of_accounts filter
89-89: 🛠️ Refactor suggestion
Verify platform connector interaction with charts_of_accounts
The test should verify how the charts_of_accounts parameter affects the expense attribute synchronization through the platform connector. Consider:
- Testing if the platform connector correctly handles the charts_of_accounts filter
- Adding assertions to verify the filtered expense attributes
tests/test_workspaces/test_view.py (2)
8-8: LGTM: Import statement is well organized
The ImportSetting model import is correctly placed with other model imports and maintains alphabetical order.
182-187: 🛠️ Refactor suggestion
Enhance test coverage for charts_of_accounts field
While the ImportSetting is created with charts_of_accounts, the test should be enhanced to:
- Verify the charts_of_accounts value after creation
- Assert that charts_of_accounts is properly included in the API response
- Test edge cases such as empty list or invalid values
Add these assertions after the ImportSetting creation:
ImportSetting.objects.create(
workspace_id=workspace.id,
import_categories=True,
import_vendors_as_merchants=True,
charts_of_accounts=['Expense']
)
+
+# Verify the created settings
+import_setting = ImportSetting.objects.get(workspace_id=workspace.id)
+assert import_setting.charts_of_accounts == ['Expense']
+
+# Test edge cases
+response = api_client.put(
+ url,
+ data={
+ 'import_settings': {
+ 'charts_of_accounts': [] # Test empty list
+ }
+ },
+ format='json'
+)
+assert response.status_code == 200Also, please verify that the test data fixtures in test_fyle/fixtures.py include the charts_of_accounts field:
tests/test_mappings/test_imports/test_modules/test_base.py (1)
40-40: Add test case for non-empty charts_of_accounts
While the current test covers the basic case with empty charts_of_accounts, we should add a test case that verifies the sync behavior with non-empty charts_of_accounts to ensure complete test coverage.
✅ Verification successful
Test coverage for charts_of_accounts is sufficient
The current test coverage is adequate because:
- All Category instantiations consistently use empty lists for charts_of_accounts parameter, which is the correct default behavior for testing basic functionality
- The actual charts_of_accounts values are properly tested at a higher level in workspace tests (
test_workspaces/test_view.py) and fixtures (test_fyle/fixtures.py), where ["Expense"] is used as a real-world value - This layered testing approach (unit tests with empty lists + integration tests with actual values) provides good coverage of the feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other test cases in the codebase that cover non-empty charts_of_accounts
rg -l 'Category\([^)]+,\s*[^)]+,\s*[^)]+,\s*\[[^]]+\]'
Length of output: 56
Script:
#!/bin/bash
# Let's try a more comprehensive search for Category instantiations
rg -A 2 "Category\("
# Also search for test files containing "charts_of_accounts" to see test coverage
rg -l "charts_of_accounts" "tests/"
Length of output: 2628
Script:
#!/bin/bash
# Let's check the implementation of Category class to understand the charts_of_accounts parameter usage
ast-grep --pattern 'class Category {
$$$
}'
# Also check the test files that mention charts_of_accounts to understand the test coverage
rg -B 2 -A 2 "charts_of_accounts" "tests/test_workspaces/test_view.py" "tests/test_fyle/fixtures.py"
Length of output: 2016
tests/test_fyle/fixtures.py (1)
273-273: Verify consistent usage across the codebase
Let's verify that the new charts_of_accounts field is consistently handled across the codebase.
Also applies to: 296-296, 327-327, 343-343, 359-359
✅ Verification successful
Field usage is consistent across the codebase
The charts_of_accounts field is properly integrated throughout the codebase:
- Defined in
ImportSettingmodel with proper ArrayField type and default value - Has a dedicated migration file
- Used consistently in import modules for category filtering
- Properly handled in workspace serializers
- Test fixtures and test cases maintain consistent usage with
["Expense"]value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of charts_of_accounts field
# Expected results: Find all related model definitions and usages
# Check model definitions
echo "Checking model definitions..."
rg -l "charts_of_accounts" --type python
# Check for field usage in views/serializers
echo "Checking field usage in views/serializers..."
ast-grep --pattern 'class $_(Serializer|ViewSet) {
$$$
charts_of_accounts
$$$
}'
# Check for field validation
echo "Checking field validation..."
rg "charts_of_accounts.*validate" --type python
Length of output: 476
Script:
#!/bin/bash
# Let's try a different approach without file type filtering
# First, find all Python files containing charts_of_accounts
echo "Files containing charts_of_accounts:"
rg -l "charts_of_accounts"
# Then check the actual usage context with surrounding lines
echo -e "\nUsage context:"
rg -C 3 "charts_of_accounts"
# Look specifically in test files
echo -e "\nTest file usage:"
fd "test.*\.py" --exec rg -l "charts_of_accounts" {} \;
# Check fixtures specifically
echo -e "\nFixture usage:"
fd "fixtures.*\.py" --exec rg -l "charts_of_accounts" {} \;
Length of output: 9086
| @@ -32,7 +32,7 @@ def test_sync_destination_attributes_categories( | |||
| ).count() | |||
| assert account_count == 0 | |||
|
|
|||
| category = Category(workspace_id, "ACCOUNT", None) | |||
| category = Category(workspace_id, "ACCOUNT", None, []) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for non-empty charts_of_accounts parameter
While the Category constructor has been updated to include the new charts_of_accounts parameter, the test only verifies behavior with an empty list. Consider adding test cases that verify the sync behavior with:
- Non-empty charts_of_accounts list
- Invalid chart of accounts values
- Edge cases (null, undefined, etc.)
Also applies to: 59-59
| @@ -270,6 +270,7 @@ | |||
| "import_settings": { | |||
| "import_categories": True, | |||
| "import_vendors_as_merchants": True, | |||
| "charts_of_accounts": ["Expense"] | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for charts_of_accounts
The test fixtures only cover the happy path with ["Expense"]. Consider adding test cases for:
- Empty array
[] - Multiple values
["Expense", "Revenue"] - Invalid values
- Null/undefined scenarios
This will ensure robust handling of the new field across different scenarios.
Add these test fixtures:
"import_settings_payload": {
"import_settings": {
"import_categories": True,
"import_vendors_as_merchants": True,
- "charts_of_accounts": ["Expense"]
},
...
},
+"import_settings_empty_charts": {
+ "import_settings": {
+ "import_categories": True,
+ "import_vendors_as_merchants": True,
+ "charts_of_accounts": []
+ },
+ ...
+},
+"import_settings_multiple_charts": {
+ "import_settings": {
+ "import_categories": True,
+ "import_vendors_as_merchants": True,
+ "charts_of_accounts": ["Expense", "Revenue"]
+ },
+ ...
+},
+"import_settings_invalid_charts": {
+ "import_settings": {
+ "import_categories": True,
+ "import_vendors_as_merchants": True,
+ "charts_of_accounts": null
+ },
+ ...
+}Also applies to: 296-296, 327-327, 343-343, 359-359
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 (1)
tests/test_workspaces/test_view.py (1)
182-187: Consider restructuring test_import_settings for better organizationThe test_import_settings function handles multiple scenarios and could benefit from:
- Breaking it down into smaller, focused test functions (e.g., test_import_settings_creation, test_import_settings_validation)
- Updating the test fixtures in test_fyle/fixtures.py to include charts_of_accounts test data
Example structure:
def test_import_settings_creation(mocker, api_client, test_connection, create_temp_workspace): """Test basic creation of import settings""" # Setup code... def test_import_settings_validation(mocker, api_client, test_connection, create_temp_workspace): """Test validation of import settings fields""" # Validation tests... def test_import_settings_api_response(mocker, api_client, test_connection, create_temp_workspace): """Test API response structure""" # API response tests...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/test_workspaces/test_view.py(2 hunks)
🔇 Additional comments (1)
tests/test_workspaces/test_view.py (1)
8-8: LGTM: Import statement is correctly placed
The ImportSetting model import is appropriately grouped with other workspace models.
| ImportSetting.objects.create( | ||
| workspace_id=workspace.id, | ||
| import_categories=True, | ||
| import_vendors_as_merchants=True, | ||
| charts_of_accounts=['Expense'] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for ImportSetting
While the ImportSetting creation is implemented, the test coverage could be improved:
- Add assertions to verify the ImportSetting object was created with correct attributes
- Add test cases for invalid charts_of_accounts values
- Verify the charts_of_accounts field in API responses
Consider adding these test cases:
# Verify ImportSetting creation
import_setting = ImportSetting.objects.get(workspace_id=workspace.id)
assert import_setting.charts_of_accounts == ['Expense']
# Test invalid charts_of_accounts
with pytest.raises(ValidationError):
ImportSetting.objects.create(
workspace_id=workspace.id,
import_categories=True,
import_vendors_as_merchants=True,
charts_of_accounts=123 # Invalid type
)
# Verify charts_of_accounts in API response
assert response.data['import_settings']['charts_of_accounts'] == ['Expense']
…ues (#173) * Feat: Add sync method for bank accounts, dimensions and dimension_values * pylint solve * minor changes * added limit for bank account and dimensions * fix test * comment resolved * Feat: Support for dimension during export Journal Entry and purchase Invoice (#174) * Feat: Support for dimension during export Journal Entry and purchase Invoice * comment resolved * added dimension support for JE and bug fix * implementation of exporting dimensions for JE and PI * bug fixed * comment resolved * flake fixed * Feat: Adding new Journal Entry changes (#175) * Feat: Adding new Journal Entry changes * comment resolved * pylint * comment line * test fixed * remove print * Feat: Add chart of accounts type when importing accounts to category in Fyle (#177) * Feat: Add chart of accounts type when importing accounts to category in Fyle * resolved comment and added sanitizing * resolved comment * remove unnessary line * comment resolved * test resolve * pylint --------- Co-authored-by: Ashwin Thanaraj <[email protected]>
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901604887942-1
Summary by CodeRabbit
Release Notes
New Features
charts_of_accounts, in import settings to enhance data handling.charts_of_accountsfunctionality.Bug Fixes
Documentation
charts_of_accountsfield to clarify its purpose.Refactor
updatemethod in the serializer to handle changes to thecharts_of_accountsfield effectively.