feat: add script for split expense grouping #676
Conversation
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several significant changes across multiple files, primarily focusing on enhancing the handling of expense-related data and tax calculations within the NetSuite integration. Key modifications include the introduction of new methods for processing taxed line items, updating existing methods to handle multiple line items, and enhancing SQL scripts for data integrity. Additionally, test suites have been updated to reflect these changes, ensuring comprehensive coverage for the new functionalities. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-script
branch
from
56f2012 to
4c13fd2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (27)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
1153-1154: Consider alternative design for NULL handlingThe
is_posted_at_nullboolean column appears to be a workaround for handling NULL values. This pattern might indicate a design smell.Consider:
- Using the
posted_atcolumn directly for NULL checks- Adding a database constraint to ensure data integrity
- Documenting the reason for this design choice if it's addressing a specific edge case
apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py (1)
16-16: Consider enhancing the help text for better clarity.The current help text "Is tax balancing enabled" is a bit terse. Consider providing more context about what tax balancing means and when it should be enabled.
- field=models.BooleanField(default=False, help_text='Is tax balancing enabled'), + field=models.BooleanField( + default=False, + help_text='Enable this option to automatically balance tax amounts across expense entries' + ),apps/fyle/migrations/0035_support_split_expense_grouping.py (3)
1-6: Remove extra blank lineThere's an unnecessary blank line at line 6.
import apps.fyle.models from django.db import migrations, models -
14-18: Consider enhancing the help text for better clarityThe current help text "Bank Transaction ID" could be more descriptive to better explain the purpose and source of this ID.
- field=models.CharField(blank=True, help_text='Bank Transaction ID', max_length=255, null=True), + field=models.CharField(blank=True, help_text='Unique identifier for the bank transaction associated with this expense', max_length=255, null=True),
13-24: Consider migration deployment strategyThis migration adds fields to existing tables, which requires careful deployment planning:
- The
bank_transaction_idaddition is safe as it's nullable- The
split_expense_groupingaddition relies on a default function and should be tested thoroughly- Consider whether this migration should be deployed separately from the application code changes
Recommendations:
- Deploy this migration during a maintenance window
- Have a rollback plan ready
- Test the migration on a copy of production data first
- Monitor the migration execution time if the tables are large
apps/mappings/models.py (2)
71-71: Consider enhancing the help text for better clarity.The help text could be more descriptive to better explain when and why tax balancing would be enabled.
Consider updating the help text to something like:
- is_tax_balancing_enabled = models.BooleanField(default=False, help_text='Is tax balancing enabled') + is_tax_balancing_enabled = models.BooleanField( + default=False, + help_text='Enable automatic balancing of tax amounts across expense entries' + )
71-73: Consider adding database constraints.Since this is a tax-related feature that could have financial implications, consider adding database-level constraints to ensure data integrity.
You might want to:
- Add a database constraint that ensures tax balancing is only enabled when
override_tax_detailsis True- Consider adding an index on
is_tax_balancing_enabledif you'll be frequently querying based on this field- Document the relationship between these tax-related fields in a model docstring or comments
Example implementation:
class Meta: db_table = 'general_mappings' constraints = [ models.CheckConstraint( check=~models.Q(is_tax_balancing_enabled=True, override_tax_details=False), name='tax_balancing_requires_override' ) ]apps/workspaces/apis/export_settings/serializers.py (1)
Based on the findings, I can now provide a final response:
Test coverage and implementation updates required for new required field
The
split_expense_groupingfield has been added as a required field in the serializer, but:
- The test fixtures in
tests/test_workspaces/test_apis/test_export_settings/fixtures.pydon't include this field in any of the test cases- The model has a default value function
get_default_split_expense_grouping()returning 'MULTIPLE_LINE_ITEM'- None of the existing test data includes this new required field
Required actions:
- Update all test fixtures to include the
split_expense_groupingfield- Add test cases to verify the field validation
- Update API documentation to reflect this new required field
🔗 Analysis chain
Line range hint
88-99: Verify test coverage and API client compatibility.Since a new required field has been added:
- Ensure all existing tests are updated to include this field
- Verify that API clients are prepared for this change
- Check if the update method properly handles this field
Let me gather more specific information about the test coverage and implementation details.
Let me check the test fixtures and model definition to get a complete picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files and API clients that might need updates # Look for test files related to export settings echo "=== Related Test Files ===" fd -e py test_export_settings # Check for existing tests using the serializer echo -e "\n=== Existing Test Usage ===" rg -A 5 "ExpenseGroupSettingsSerializer" -g "test_*.py" # Look for API client implementations echo -e "\n=== API Client Usage ===" rg -l "expense_group_settings" --type pyLength of output: 1013
Script:
#!/bin/bash # Check test coverage for the new field echo "=== Test Coverage for split_expense_grouping ===" rg -B 5 -A 5 "split_expense_grouping" tests/test_workspaces/test_apis/test_export_settings/test_views.py # Check serializer usage in views echo -e "\n=== View Implementation ===" rg -B 5 -A 5 "ExpenseGroupSettingsSerializer" apps/workspaces/apis/export_settings/views.py # Check model definition echo -e "\n=== Model Definition ===" ast-grep --pattern 'class ExpenseGroupSettings($$$)'Length of output: 494
Script:
#!/bin/bash # Check test fixtures for export settings echo "=== Test Fixtures ===" rg -B 5 -A 10 "expense_group_settings" tests/test_workspaces/test_apis/test_export_settings/fixtures.py # Check model definition in a different way echo -e "\n=== Model Definition ===" rg -B 5 -A 10 "class ExpenseGroupSettings" apps/workspaces/models.py apps/*/models.py # Check view implementation for POST/PUT handling echo -e "\n=== View POST/PUT Implementation ===" rg -B 5 -A 10 "def (post|put)" apps/workspaces/apis/export_settings/views.pyLength of output: 4739
tests/test_fyle/test_models.py (2)
134-168: Enhance test robustness and documentationWhile the test structure is good, consider these improvements:
- Add more descriptive assertion messages explaining the expected behavior
- Document the test data structure and requirements
- Add edge cases (e.g., empty expense list)
def test_split_expense_grouping_with_no_bank_transaction_id(db, update_config_for_split_expense_grouping): ''' - Test for grouping of 2 expenses with no bank transaction id + Test expense grouping behavior when bank_transaction_id is None + + Test data requirements: + - First 2 expenses from ccc_split_expenses fixture + - Expenses should have different expense IDs + + Scenarios tested: + 1. SINGLE_LINE_ITEM: Should create separate groups + 2. MULTIPLE_LINE_ITEM: Should create separate groups ''' workspace_id = 1 # Update settings configuration = Configuration.objects.get(workspace_id=workspace_id) expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) update_config_for_split_expense_grouping(configuration, expense_group_settings) # Get reference to expense objects expenses = data['ccc_split_expenses'][:2] + # Verify test data meets requirements + assert expenses[0]['id'] != expenses[1]['id'], 'Test requires expenses with different IDs' for expense in expenses: expense['bank_transaction_id'] = None Expense.create_expense_objects(expenses, workspace_id=workspace_id) expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) - assert len(expense_objects) == 2, f'Expected 2 expenses, got {len(expense_objects)}' + assert len(expense_objects) == 2, 'Setup failed: Expected 2 test expenses to be created'
209-245: Refactor for better maintainabilityConsider these improvements:
- Use parameterized testing to reduce code duplication
- Extract test data setup into helper functions
- Use named constants instead of magic numbers
Example refactor:
import pytest # Constants for test data SAMPLE_BANK_TRANSACTION_ID_1 = 'sample_1' SAMPLE_BANK_TRANSACTION_ID_2 = 'sample_2' @pytest.mark.parametrize('test_scenario', [ { 'name': 'no_bank_transaction_id', 'expense_count': 2, 'bank_transaction_ids': [None, None], 'expected_single_line_groups': 2, 'expected_multiple_line_groups': 2 }, { 'name': 'mixed_transaction_ids', 'expense_count': 3, 'bank_transaction_ids': [SAMPLE_BANK_TRANSACTION_ID_1, SAMPLE_BANK_TRANSACTION_ID_1, SAMPLE_BANK_TRANSACTION_ID_2], 'expected_single_line_groups': 3, 'expected_multiple_line_groups': 2 }, # Add more scenarios... ]) def test_split_expense_grouping(db, update_config_for_split_expense_grouping, test_scenario): workspace_id = 1 # Setup configuration = Configuration.objects.get(workspace_id=workspace_id) expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) update_config_for_split_expense_grouping(configuration, expense_group_settings) # Prepare test data expenses = data['ccc_split_expenses'][:test_scenario['expense_count']] for expense, bank_transaction_id in zip(expenses, test_scenario['bank_transaction_ids']): expense['bank_transaction_id'] = bank_transaction_id # Test execution and verification # ... rest of the test logictests/test_netsuite/conftest.py (1)
308-324: Improve tax destination attributes fixtureSeveral improvements needed for the new fixture:
- Add docstring explaining the purpose and usage
- Consider more descriptive naming (e.g.,
create_tax_items_for_testing)- Avoid potential ID conflicts
Example improvement:
@pytest.fixture def create_tax_items_for_testing(db): """ Creates test tax items for multiple workspaces. Each workspace gets a unique tax item with a 5% tax rate. Used by tests that verify tax calculation behavior. """ tax_items = [] for workspace_id in (1, 2, 49): tax_items.append( DestinationAttribute.objects.create( attribute_type='TAX_ITEM', value=f'Test Tax Item - Workspace {workspace_id}', destination_id=f'test_tax_item_{workspace_id}', active=True, detail={'tax_rate': 5.0}, workspace_id=workspace_id ) ) return tax_itemsapps/fyle/models.py (3)
215-217: Add docstring to the default function.Consider adding a docstring to explain the purpose and default value of the split expense grouping.
def get_default_split_expense_grouping(): + """ + Returns the default split expense grouping setting. + + Returns: + str: Default value 'MULTIPLE_LINE_ITEM' for split expense grouping + """ return 'MULTIPLE_LINE_ITEM'
243-247: Enhance help text for better clarity.The help text could be more descriptive to better explain the purpose and impact of this setting.
split_expense_grouping = models.CharField( max_length=100, default=get_default_split_expense_grouping, choices=SPLIT_EXPENSE_GROUPING, - help_text='specify line items for split expenses grouping' + help_text='Specifies whether split expenses should be grouped as single or multiple line items in credit card charges' )
421-458: Consider extracting split expense grouping logic into a separate method.The current implementation mixes multiple concerns and has deep nesting. Consider extracting the split expense grouping logic into a separate method for better readability and maintainability.
if corporate_credit_card_expenses: - # Group split Credit Card Charges by `bank_transaction_id` - if ( - configuration.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' and - expense_group_settings.split_expense_grouping == 'MULTIPLE_LINE_ITEM' - ): - ccc_expenses_without_bank_transaction_id = list( - filter(lambda expense: not expense.bank_transaction_id, corporate_credit_card_expenses) - ) - - ccc_expenses_with_bank_transaction_id = list( - filter(lambda expense: expense.bank_transaction_id, corporate_credit_card_expenses) - ) - - if ccc_expenses_without_bank_transaction_id: - groups_without_bank_transaction_id = _group_expenses( - ccc_expenses_without_bank_transaction_id, corporate_credit_card_expense_group_field, workspace_id - ) - - expense_groups.extend(groups_without_bank_transaction_id) - - if ccc_expenses_with_bank_transaction_id: - split_expense_group_fields = [ - field for field in corporate_credit_card_expense_group_field - if field not in ('expense_id', 'expense_number') - ] - split_expense_group_fields.append('bank_transaction_id') - - groups_with_bank_transaction_id = _group_expenses( - ccc_expenses_with_bank_transaction_id, split_expense_group_fields, workspace_id - ) - expense_groups.extend(groups_with_bank_transaction_id) - - else: - corporate_credit_card_expense_groups = _group_expenses( - corporate_credit_card_expenses, corporate_credit_card_expense_group_field, workspace_id) - - expense_groups.extend(corporate_credit_card_expense_groups) + expense_groups.extend( + _group_corporate_credit_card_expenses( + corporate_credit_card_expenses, + corporate_credit_card_expense_group_field, + configuration, + expense_group_settings, + workspace_id + ) + ) +def _group_corporate_credit_card_expenses(expenses, group_fields, configuration, settings, workspace_id): + """ + Groups corporate credit card expenses based on configuration and settings. + + Args: + expenses (list): List of corporate credit card expenses + group_fields (list): Fields to group by + configuration (Configuration): Workspace configuration + settings (ExpenseGroupSettings): Expense group settings + workspace_id (int): Workspace ID + + Returns: + list: List of expense groups + """ + if ( + configuration.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' and + settings.split_expense_grouping == 'MULTIPLE_LINE_ITEM' + ): + return _group_split_expenses(expenses, group_fields, workspace_id) + + return _group_expenses(expenses, group_fields, workspace_id) + +def _group_split_expenses(expenses, group_fields, workspace_id): + """ + Groups split expenses based on bank transaction ID. + """ + expense_groups = [] + + expenses_by_bank_id = { + True: list(filter(lambda e: e.bank_transaction_id, expenses)), + False: list(filter(lambda e: not e.bank_transaction_id, expenses)) + } + + if expenses_by_bank_id[False]: + expense_groups.extend(_group_expenses( + expenses_by_bank_id[False], group_fields, workspace_id + )) + + if expenses_by_bank_id[True]: + split_fields = [ + field for field in group_fields + if field not in ('expense_id', 'expense_number') + ] + split_fields.append('bank_transaction_id') + + expense_groups.extend(_group_expenses( + expenses_by_bank_id[True], split_fields, workspace_id + )) + + return expense_groupsapps/netsuite/models.py (1)
118-129: Consider adding error handling and documentation.The changes improve the code by checking tax settings before attempting to get mappings. However, consider these enhancements:
- Add error handling for database queries
- Add a docstring to document the function's purpose, parameters, and return value
def get_tax_item_id_or_none(expense_group: ExpenseGroup, general_mapping: GeneralMapping, lineitem: Expense = None): + """ + Get tax item ID based on workspace settings and mappings. + + Args: + expense_group (ExpenseGroup): The expense group + general_mapping (GeneralMapping): General mapping settings + lineitem (Expense, optional): The expense line item. Defaults to None. + + Returns: + str: Tax code if found, None otherwise + """ tax_code = None try: tax_setting: MappingSetting = MappingSetting.objects.filter( workspace_id=expense_group.workspace_id, destination_field='TAX_ITEM' ).first() if tax_setting: mapping = get_tax_group_mapping(lineitem, expense_group.workspace_id) if mapping: tax_code = mapping.destination.destination_id else: tax_code = general_mapping.default_tax_code_id + except Exception as e: + logger.error('Error while getting tax item ID: %s', str(e)) return tax_codetests/test_netsuite/test_connector.py (1)
22-22: Avoid hardcodingworkspace_idin testsUsing
workspace_id=1directly in tests can make them less flexible and may lead to issues if the workspace ID changes. Consider parameterizing theworkspace_idor using fixtures to provide the workspace ID dynamically.tests/test_netsuite/test_tasks.py (5)
210-212: Variable naming: Use singular form for loop variableIn the loop
for expenses in expense_group.expenses.all():, consider renaming the loop variable toexpenseto reflect that each item is a single expense.Apply this diff to improve code readability:
-for expenses in expense_group.expenses.all(): - expenses.workspace_id = 2 - expenses.save() +for expense in expense_group.expenses.all(): + expense.workspace_id = 2 + expense.save()
Line range hint
258-258: Incorrect assignment toconfiguration.auto_map_employeesThe
auto_map_employeesfield expects a string value like'EMAIL'or'NAME', but it's being assigned a booleanTrue. Assigning a boolean may cause unexpected behavior. Please update it to the appropriate string value.Apply this diff to fix the issue:
-configuration.auto_map_employees = True +configuration.auto_map_employees = 'EMAIL'🧰 Tools
🪛 Ruff (0.7.0)
175-175: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
Line range hint
346-346: Incorrect assignment toconfiguration.auto_map_employeesThe
auto_map_employeesfield expects a string value like'EMAIL'or'NAME', but it's being assigned a booleanTrue. Assigning a boolean may cause unexpected behavior. Please update it to the appropriate string value.Apply this diff to fix the issue:
-configuration.auto_map_employees = True +configuration.auto_map_employees = 'EMAIL'🧰 Tools
🪛 Ruff (0.7.0)
175-175: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
Line range hint
474-474: Incorrect assignment toconfiguration.auto_map_employeesThe
auto_map_employeesfield expects a string value like'EMAIL'or'NAME', but it's being assigned a booleanTrue. Assigning a boolean may cause unexpected behavior. Please update it to the appropriate string value.Apply this diff to fix the issue:
-configuration.auto_map_employees = True +configuration.auto_map_employees = 'EMAIL'🧰 Tools
🪛 Ruff (0.7.0)
175-175: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
Line range hint
203-205: Use dynamic timestamps instead of hardcoded datesHardcoding timestamps can lead to flaky tests or issues when the hardcoded date becomes outdated. Consider using
timezone.now()or mocking the datetime for consistent test results.Apply this diff to improve:
-from datetime import datetime +from django.utils import timezone LastExportDetail.objects.create(workspace_id=2, export_mode='MANUAL', total_expense_groups_count=2, - successful_expense_groups_count=0, failed_expense_groups_count=0, last_exported_at='2023-07-07 11:57:53.184441+00', - created_at='2023-07-07 11:57:53.184441+00', updated_at='2023-07-07 11:57:53.184441+00') + successful_expense_groups_count=0, failed_expense_groups_count=0, last_exported_at=timezone.now(), + created_at=timezone.now(), updated_at=timezone.now())🧰 Tools
🪛 Ruff (0.7.0)
175-175: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
apps/netsuite/connector.py (6)
1165-1235: Refactorhandle_taxed_line_itemsto Reduce Code DuplicationThe
handle_taxed_line_itemsmethod contains similar code blocks in both theifandelsebranches when setting values forbase_lineand appending to thelineslist. This duplication can make the code harder to maintain and understand. Consider refactoring common code into helper functions or restructure the method to eliminate redundancy.
Line range hint
1549-1568: Improve Exception Handling inpost_billThe
post_billmethod catches exceptions and checks the error message string to decide whether to retry with a different date. Comparing error messages as strings is fragile and can break if the message changes. Consider using specific exception types or error codes for more reliable error handling.
1183-1184: Clarify Tax Calculation LogicThe calculation for
expected_tax_amountat line 1183 may be complex to read and understand. Consider simplifying the expression or adding comments to explain the calculation steps for better readability.
Line range hint
1628-1674: Ensure Robust Error Handling inpost_credit_card_chargeIn the
post_credit_card_chargemethod, raw HTTP responses are parsed manually, which may lead to unhandled exceptions if the response format is unexpected. Use a try-except block when parsing JSON responses to handleJSONDecodeErrorand other potential parsing issues gracefully.
1166-1172: Update Docstring inhandle_taxed_line_itemsThe docstring mentions a parameter
is_credit_card_chargewhich is not present in the method signature. Update the docstring to reflect the actual parameters and provide accurate documentation.Apply this diff to correct the docstring:
1166 def handle_taxed_line_items(self, base_line, line, workspace_id, export_module, general_mapping: GeneralMapping): 1167 """ 1168 Handle line items where tax is applied or modified by the user. 1169 :param base_line: The base line item template that will be modified. 1170 :param line: The original line with tax and amount information. -1171 :param is_credit_card_charge: Boolean flag to differentiate between credit card charges and other transactions. +1171 :param workspace_id: The workspace ID. +1172 :param export_module: The export module being used ('BILL', 'EXPENSE_REPORT', etc.). +1173 :param general_mapping: The general mapping configuration. 1174 :return: List of lines (taxed and/or untaxed). 1175 """
Line range hint
1702-1730: Refactor Error Handling inpost_credit_card_chargeThe error handling logic for retrying the request with a different date is duplicated. Extract this logic into a helper function to reduce code duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
apps/fyle/migrations/0035_support_split_expense_grouping.py(1 hunks)apps/fyle/models.py(10 hunks)apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py(1 hunks)apps/mappings/models.py(1 hunks)apps/netsuite/connector.py(28 hunks)apps/netsuite/models.py(1 hunks)apps/netsuite/tasks.py(8 hunks)apps/workspaces/apis/export_settings/serializers.py(2 hunks)scripts/sql/scripts/030-default-split-expense-grouping-for-old-orgs.sql(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(9 hunks)tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/fixtures.py(13 hunks)tests/test_fyle/test_models.py(2 hunks)tests/test_netsuite/conftest.py(5 hunks)tests/test_netsuite/fixtures.py(3 hunks)tests/test_netsuite/test_connector.py(11 hunks)tests/test_netsuite/test_tasks.py(2 hunks)tests/test_workspaces/test_apis/test_export_settings/fixtures.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/sql/scripts/030-default-split-expense-grouping-for-old-orgs.sql
- tests/test_fyle/fixtures.py
🔇 Additional comments (44)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (6)
7993-7997: LGTM! Migration sequence is well-organized
The migration sequence shows a logical progression of changes:
- Tax balancing feature
- Posted at NULL handling
- Task log modifications
- Configuration changes
- Split expense grouping support
11584-11587: LGTM! Test data is comprehensive
The test data:
- Covers various expense grouping scenarios
- Includes both reimbursable and corporate card expenses
- Provides tax-related test cases
- Maintains referential integrity
Also applies to: 11639-11647, 11663-11666
11909-11909: LGTM! Sequence value matches migrations
The sequence value update to 205 corresponds to the number of migrations added.
1445-1446: Verify tax handling implementation
New tax-related columns have been added. Ensure that:
- Tax calculations comply with accounting standards
- The tax balancing feature is properly documented
- Test cases cover various tax scenarios
✅ Verification successful
Tax handling implementation is properly tested and documented
The codebase shows comprehensive implementation and testing of the tax balancing feature:
- Tax balancing is implemented in
apps/netsuite/connector.pywith proper validation of tax amounts - Test coverage includes various scenarios:
- Expense reports (
test_construct_expense_report_with_tax_balancing) - Bills (
test_construct_bill_item_for_tax_balancing) - Journal entries (
test_construct_journal_entry_with_tax_balancing) - Credit card charges (
test_contruct_credit_card_charge_with_tax_balancing)
- Expense reports (
- Each test verifies both enabled and disabled states of tax balancing
- The feature is properly documented with help text in the model definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tax-related implementation and tests
# Search for tax balancing implementation
rg "tax_balancing"
# Search for related test files
fd -e py test_ | xargs rg "tax_balancing|tax_code"
Length of output: 4630
962-963: Verify migration strategy for new NOT NULL column
The new split_expense_grouping column is added as NOT NULL without a default value. This could cause issues during migration if existing rows exist.
5-5: Verify PostgreSQL version downgrade
The PostgreSQL version has been downgraded from 15.9 to 15.7. This could potentially introduce compatibility issues.
apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py (1)
1-18: LGTM! Migration structure follows Django conventions.
The migration is well-structured with appropriate dependencies and default values, ensuring backward compatibility.
apps/fyle/migrations/0035_support_split_expense_grouping.py (1)
19-23: Verify the default function and choices implementation
A few considerations for this field:
- The default function
get_default_split_expense_groupingis imported from models, which could potentially cause circular import issues - The choices would be better defined as constants in the models file
- Since this is a non-nullable field being added to an existing table, verify that the default function handles all existing records correctly
Let's verify the implementation:
✅ Verification successful
No issues found with the split_expense_grouping field implementation
The implementation is correct and well-structured:
- The default function
get_default_split_expense_grouping()is properly defined in the same models file and returns 'MULTIPLE_LINE_ITEM' - The choices are defined as a tuple constant
SPLIT_EXPENSE_GROUPINGat the module level - The field is properly implemented in the ExpenseGroupSettings model with appropriate choices and default value
- The field is used correctly in the expense grouping logic within
create_expense_groups_by_report_id_fund_source()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the default function implementation
ast-grep --pattern 'def get_default_split_expense_grouping'
# Check if choices are defined as constants
ast-grep --pattern 'SPLIT_EXPENSE_GROUPING_CHOICES'
# Check existing records that might be affected
echo "SELECT COUNT(*) FROM expense_group_settings;" | psql
Length of output: 269
Script:
#!/bin/bash
# Let's check the models.py file content to verify the implementation
fd "models.py" --exec cat {} \; 2>/dev/null
# Also check if there are any imports of the default function
rg "get_default_split_expense_grouping" -A 2 -B 2
Length of output: 105654
Script:
#!/bin/bash
# Let's check the models.py file specifically in the fyle app directory
fd "models.py" --type f --full-path --base-directory apps/fyle/ --exec cat {} \;
Length of output: 28281
tests/test_fyle/conftest.py (3)
31-33: LGTM! Fixture structure follows best practices.
The fixture is properly defined with database dependency and follows the factory pattern by returning a configuration function.
34-37: Verify configuration values match database migration.
The hardcoded values ('CREDIT CARD CHARGE' and 'SINGLE_LINE_ITEM') should align with the values set in the SQL migration script mentioned in the PR.
✅ Verification successful
Let me gather more information about the 'CREDIT CARD CHARGE' value as well.
Configuration values are consistent with the codebase
The verification confirms that both configuration values are valid and consistently used across the codebase:
- 'SINGLE_LINE_ITEM' is the default value set in migration
030-default-split-expense-grouping-for-old-orgs.sql - 'CREDIT CARD CHARGE' is a valid configuration value used throughout the codebase, including SQL fixtures, models, and business logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the configuration values in the test fixture match the SQL migration
# Search for the SQL migration file and its content
rg -l "split_expense_grouping" --type sql
rg "SINGLE_LINE_ITEM" --type sql -A 5 -B 5
Length of output: 663
Script:
#!/bin/bash
# Search for 'CREDIT CARD CHARGE' in SQL files and Python files
rg "CREDIT_CARD_CHARGE|CREDIT CARD CHARGE" --type sql -A 5 -B 5
rg "corporate_credit_card_expenses_object.*CREDIT" --type py -A 2 -B 2
# Also check for any enum or constant definitions that might define these values
ast-grep --pattern 'class $_ (Enum|Choices|TextChoices) {
$$$
CREDIT
$$$
}'
Length of output: 9330
31-47: Verify test coverage for the new configuration.
According to the AI summary, this fixture should be used in test cases within test_models.py to validate both SINGLE_LINE_ITEM and MULTIPLE_LINE_ITEM groupings.
✅ Verification successful
Test coverage for split expense grouping is comprehensive
The fixture is properly used in three test cases in test_models.py that cover different scenarios:
- Expenses with no bank transaction ID
- Mix of expenses with same and different bank transaction IDs
- Multiple pairs of expenses with same bank transaction IDs
Each test verifies both SINGLE_LINE_ITEM configuration (set by the fixture) and includes assertions to validate the expense grouping behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the fixture is properly used in tests
# Search for test cases using this fixture
rg "update_config_for_split_expense_grouping" --type python tests/
Length of output: 97
Script:
#!/bin/bash
# Search for test cases using this fixture without file type restriction
rg "update_config_for_split_expense_grouping" tests/
# Also search for split expense grouping related tests
rg "SINGLE_LINE_ITEM|MULTIPLE_LINE_ITEM" tests/
# Look for test files that might contain related tests
fd "test_.*\.py" tests/
Length of output: 4713
Script:
#!/bin/bash
# Get the content of test cases using this fixture to verify coverage
rg -B 2 -A 10 "test_split_expense_grouping" tests/test_fyle/test_models.py
Length of output: 1589
Script:
#!/bin/bash
# Get more context around these test cases to see the actual test logic
rg -B 2 -A 25 "test_split_expense_grouping" tests/test_fyle/test_models.py
Length of output: 3992
apps/mappings/models.py (1)
73-73: Verify the impact of adding related_name.
The addition of related_name='general_mappings' to the workspace field is a good practice, but we should verify that no existing code relies on the default reverse accessor name.
✅ Verification successful
Based on the search results, I can now generate the final response:
The addition of related_name='general_mappings' is safe to implement.
The codebase search shows that all interactions with the GeneralMapping model are done through direct model queries (e.g., GeneralMapping.objects.get/filter) and not through the reverse relationship from Workspace. There are no instances of using the default reverse accessor name generalmapping through the workspace model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of the default reverse accessor (generalmapping)
# which might break with the new related_name
rg -l "workspace\.generalmapping\."
# Also search for potential raw SQL queries that might be affected
rg -l "JOIN general_mappings"
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search to catch any potential usage patterns
# Search for generalmapping in any context
rg -i "generalmapping"
# Search for the model name itself to understand usage patterns
rg -i "class GeneralMapping"
# Search for any imports of GeneralMapping
rg -i "from .+ import .+GeneralMapping"
# Look for potential model references in migrations
fd -e py -x grep -l "GeneralMapping" {} \; migrations/
# Check for any Django ORM related queries
rg "\.objects\.filter.*workspace"
rg "\.objects\.get.*workspace"
Length of output: 93422
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (2)
29-30: LGTM! New field added for split expense grouping configuration.
The addition of split_expense_grouping with value 'MULTIPLE_LINE_ITEM' aligns with the PR's objective of implementing split expense grouping functionality.
29-30: Verify if missing values test cases need updating.
While the split_expense_grouping field is correctly added to both main test structures, it's not present in the "missing values" test scenarios. Please verify if this is intentional or if these test cases should be updated to cover the new field's behavior when missing.
Also applies to: 78-79
apps/workspaces/apis/export_settings/serializers.py (1)
98-99: LGTM!
The field is correctly added to the Meta.fields list, maintaining consistency with the field declarations.
tests/test_fyle/test_models.py (1)
146-146: Verify test data fixture contents
The tests rely heavily on the ccc_split_expenses fixture data, but there's no verification of its structure or content requirements.
Also applies to: 183-183, 221-221
tests/test_netsuite/conftest.py (1)
Line range hint 1-324: Verify test coverage for split expense grouping
While the changes add support for tax handling, I don't see explicit test coverage for the split expense grouping feature mentioned in the PR title.
Let's check for related test files:
apps/fyle/models.py (2)
28-29: LGTM! Constants are well-defined and properly formatted.
The additions to ALLOWED_FIELDS and the new SPLIT_EXPENSE_GROUPING constant follow Django conventions and are self-documenting.
Also applies to: 76-77
109-109: LGTM! Field definition follows best practices.
The bank_transaction_id field is properly defined with appropriate constraints and clear help text.
tests/test_netsuite/fixtures.py (3)
5-31: LGTM! Well-structured tax details test data.
The new tax list detail structure provides comprehensive test coverage for tax calculations with multiple scenarios. The mock data follows NetSuite's API format and includes all necessary fields.
994-999: LGTM! Proper tax code structure for journal entries.
The tax code structure follows NetSuite's API format and includes all required fields.
1205-1208: LGTM! Enhanced custom field structure.
The addition of the type field to custom fields makes the test data more accurate and consistent with NetSuite's API format.
Also applies to: 1215-1217
tests/test_netsuite/test_connector.py (10)
26-26: Ensure method signature matches implementation
The method _NetSuiteConnector__construct_expense_report now includes general_mapping as an additional parameter. Ensure that this change is reflected consistently across all calls to this method and that the method implementation accepts this parameter.
82-82: Avoid hardcoding workspace_id in tests
Similar to previous comments, avoid using workspace_id=1 directly. Use parameterization or fixtures to provide flexibility and prevent potential issues with hardcoded values.
85-85: Ensure method signature matches implementation
The method _NetSuiteConnector__construct_bill now includes general_mapping as an additional parameter. Confirm that all calls and implementations are updated accordingly.
115-156: Ensure test isolation when modifying shared objects
In test_construct_bill_item_for_tax_balancing, general_mapping.is_tax_balancing_enabled is modified and saved. This modification may impact other tests due to shared state. Use fixtures or mocks to create isolated instances of general_mapping for testing purposes.
162-165: Ensure method signature updates are consistent
The method _NetSuiteConnector__construct_journal_entry now accepts general_mapping as an additional parameter. Verify that all method calls and implementations are updated to reflect this change.
Also applies to: 174-174
234-284: Ensure test isolation when modifying shared objects
test_construct_journal_entry_with_tax_balancing modifies general_mapping.is_tax_balancing_enabled and saves it. This can cause side effects in other tests. Use isolated instances or mock objects to prevent interference between tests.
289-293: Ensure method signature matches implementation
The method _NetSuiteConnector__construct_credit_card_charge now includes general_mapping as an additional parameter. Ensure all calls to this method are updated accordingly.
301-340: Ensure test isolation when modifying shared objects
In test_contruct_credit_card_charge_with_tax_balancing, the test modifies and saves general_mapping.is_tax_balancing_enabled. This may affect other tests due to shared state. Consider using fixtures or mocks to prevent shared state modifications.
853-853: Avoid hardcoding workspace_id in tests
Again, avoid using hardcoded workspace_id values in tests. Use parameterization or fixtures to provide the workspace ID.
1023-1070: Review the construction of tax details in test_constructs_tax_details_list_for_multiple_items
The test test_constructs_tax_details_list_for_multiple_items introduces a complex setup with mocked objects. Ensure that:
- All mock objects are correctly configured.
- The test accurately reflects real-world usage scenarios.
- Assertions are comprehensive to validate the behavior thoroughly.
apps/netsuite/tasks.py (9)
329-331: Updates to 'construct_lines' function calls are appropriate
Including the general_mappings parameter in the construct_lines function calls enhances the flexibility and aligns with the updates made for handling general mappings.
340-340: Proper inclusion of 'general_mappings' in 'CREATING_BILL' construct
Passing general_mappings.override_tax_details and general_mappings to the construct_lines function ensures that tax details and general mappings are correctly considered during bill creation.
344-344: 'construct_lines' function call updated correctly
The addition of general_mappings to the function call for constructing lines is appropriate and maintains consistency across different export types.
490-490: 'post_bill' method updated with 'general_mappings'
Passing general_mappings to the post_bill method allows for enhanced handling of general mappings during bill posting.
570-570: Inclusion of 'general_mappings' in 'post_credit_card_charge'
Adding the general_mappings parameter to the post_credit_card_charge method call is appropriate for accurate processing of credit card charges.
615-615: Retrieval of 'general_mapping' object is appropriate
Fetching general_mapping using GeneralMapping.objects.get() aligns with the need to access general mapping configurations for expense reports.
637-637: 'post_expense_report' method call updated appropriately
Including general_mapping in the post_expense_report method call ensures that general mappings are considered during expense report posting.
680-680: Retrieval of 'general_mapping' object is consistent
Consistently fetching general_mapping maintains uniformity across different transaction types and ensures necessary configurations are applied.
703-703: 'post_journal_entry' method updated with 'general_mapping'
Passing general_mapping to the post_journal_entry method enhances the handling of journal entries by incorporating general mapping configurations.
apps/netsuite/connector.py (3)
1236-1257: Confirm Duplicate Attachment Links in prepare_custom_segments
In prepare_custom_segments, the same attachment link is added twice with different script IDs (custcolfyle_receipt_link and custcolfyle_receipt_link_2). Verify if both custom fields are intended to hold the same value. If not, ensure that each field is assigned the correct value.
Line range hint 2271-2275: Verify the Impact of Changing tranDate
When handling exceptions in post_journal_entry, the transaction date is set to the first day of the current month. Changing financial transaction dates can have accounting implications. Confirm that adjusting tranDate in this way complies with accounting policies and does not affect financial reporting.
Line range hint 1988-2080: Validate Parameters in construct_journal_entry_lineitems
The method construct_journal_entry_lineitems uses parameters credit and debit to determine the type of journal entry line. Ensure that these parameters are being provided correctly when the method is called and that their usage within the method aligns with their intended purpose.
Run the following script to check all calls to construct_journal_entry_lineitems:
✅ Verification successful
Let me analyze the method calls to understand the parameter usage better.
Parameters in construct_journal_entry_lineitems are being used correctly
The method is called in two scenarios with proper parameter usage:
- For credit lines: Called with
credit='Credit'anddebit=None - For debit lines: Called with
debit='Debit'andcredit=None(default)
The implementation correctly handles both cases:
- When
creditis provided, it setsbase_line['credit'] = line.amountandbase_line['debit'] = None - When
debitis provided, it handles tax items and setsbase_line['debit'] = line.amount
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `construct_journal_entry_lineitems` and check parameter usage.
rg "construct_journal_entry_lineitems\(" -A 3 apps/netsuite/
Length of output: 1073
Script:
#!/bin/bash
# Get more context around the method calls to understand the flow
rg "construct_journal_entry_lineitems" -B 5 -A 10 apps/netsuite/connector.py
Length of output: 1576
tests/test_fyle/conftest.py
Outdated
| expense_group_settings.corporate_credit_card_expense_group_fields = [ | ||
| 'expense_id', | ||
| 'claim_number', | ||
| 'fund_source', | ||
| 'employee_email', | ||
| 'report_id', | ||
| 'spent_at', | ||
| 'report_id' | ||
| ] |
There was a problem hiding this comment.
Remove duplicate 'report_id' field.
The corporate_credit_card_expense_group_fields list contains 'report_id' twice (at lines 42 and 44).
Apply this diff to fix the duplicate field:
expense_group_settings.corporate_credit_card_expense_group_fields = [
'expense_id',
'claim_number',
'fund_source',
'employee_email',
'report_id',
'spent_at',
- 'report_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.
| expense_group_settings.corporate_credit_card_expense_group_fields = [ | |
| 'expense_id', | |
| 'claim_number', | |
| 'fund_source', | |
| 'employee_email', | |
| 'report_id', | |
| 'spent_at', | |
| 'report_id' | |
| ] | |
| expense_group_settings.corporate_credit_card_expense_group_fields = [ | |
| 'expense_id', | |
| 'claim_number', | |
| 'fund_source', | |
| 'employee_email', | |
| 'report_id', | |
| 'spent_at' | |
| ] |
| @@ -85,6 +85,7 @@ class ExpenseGroupSettingsSerializer(serializers.ModelSerializer): | |||
| corporate_credit_card_expense_group_fields = serializers.ListField(allow_null=True, required=False) | |||
| ccc_export_date_type = serializers.CharField(allow_null=True, allow_blank=True, required=False) | |||
| ccc_expense_state = serializers.CharField(allow_null=True, allow_blank=True, required=False) | |||
| split_expense_grouping = serializers.CharField(allow_null=False, allow_blank=False, required=True) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation constraints and documentation for the new required field.
The new split_expense_grouping field might benefit from:
- A default value to maintain backward compatibility with existing API clients
- Validation constraints (choices) to restrict valid values
- Docstring explaining the field's purpose and valid values
Consider updating the field definition like this:
- split_expense_grouping = serializers.CharField(allow_null=False, allow_blank=False, required=True)
+ split_expense_grouping = serializers.CharField(
+ allow_null=False,
+ allow_blank=False,
+ required=True,
+ default='SINGLE_LINE_ITEM', # or appropriate default
+ choices=['SINGLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM'], # adjust based on valid values
+ help_text='Controls how expenses are grouped: SINGLE_LINE_ITEM or MULTIPLE_LINE_ITEM'
+ )Committable suggestion skipped: line range outside the PR's diff.
tests/test_fyle/test_models.py
Outdated
| def test_split_expense_grouping_with_same_and_different_ids(db, update_config_for_split_expense_grouping): | ||
| ''' | ||
| Test for grouping of 2 expenses with the same bank transaction id, | ||
| and one expense with a different bank transaction id | ||
| ''' | ||
| workspace_id = 1 | ||
|
|
||
| # Update settings | ||
| configuration = Configuration.objects.get(workspace_id=workspace_id) | ||
| expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) | ||
| update_config_for_split_expense_grouping(configuration, expense_group_settings) | ||
|
|
||
| # Get reference to expense objects | ||
| expenses = data['ccc_split_expenses'][:3] | ||
| expenses[0]['bank_transaction_id'] = 'sample_1' | ||
| expenses[1]['bank_transaction_id'] = 'sample_1' | ||
| expenses[2]['bank_transaction_id'] = 'sample_2' | ||
|
|
||
| Expense.create_expense_objects(expenses, workspace_id=workspace_id) | ||
| expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) | ||
|
|
||
| assert len(expense_objects) == 3, f'Expected 3 expenses, got {len(expense_objects)}' | ||
|
|
||
| # Test for SINGLE_LINE_ITEM split expense grouping | ||
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | ||
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) | ||
| assert len(groups) == 3, f'Expected 3 groups, got {len(groups)}' | ||
| old_count = len(groups) | ||
|
|
||
| # Test for MULTIPLE_LINE_ITEM split expense grouping | ||
| expense_group_settings.split_expense_grouping = 'MULTIPLE_LINE_ITEM' | ||
| expense_group_settings.save() | ||
|
|
||
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | ||
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]).distinct() | ||
|
|
||
| assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add group content verification
The test verifies group counts but should also validate that expenses are grouped correctly based on their bank_transaction_ids.
ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id)
groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]).distinct()
- assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}'
+ new_groups = groups[old_count:]
+ assert len(new_groups) == 2, 'Expected 2 new groups to be created'
+
+ # Verify group contents
+ for group in new_groups:
+ expenses_in_group = group.expenses.all()
+ if len(expenses_in_group) == 2:
+ # Group with same bank_transaction_id
+ assert all(exp.bank_transaction_id == 'sample_1' for exp in expenses_in_group), \
+ 'Expenses with same bank_transaction_id should be grouped together'
+ else:
+ # Single expense group
+ assert expenses_in_group[0].bank_transaction_id == 'sample_2', \
+ 'Single expense should have different bank_transaction_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.
| def test_split_expense_grouping_with_same_and_different_ids(db, update_config_for_split_expense_grouping): | |
| ''' | |
| Test for grouping of 2 expenses with the same bank transaction id, | |
| and one expense with a different bank transaction id | |
| ''' | |
| workspace_id = 1 | |
| # Update settings | |
| configuration = Configuration.objects.get(workspace_id=workspace_id) | |
| expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) | |
| update_config_for_split_expense_grouping(configuration, expense_group_settings) | |
| # Get reference to expense objects | |
| expenses = data['ccc_split_expenses'][:3] | |
| expenses[0]['bank_transaction_id'] = 'sample_1' | |
| expenses[1]['bank_transaction_id'] = 'sample_1' | |
| expenses[2]['bank_transaction_id'] = 'sample_2' | |
| Expense.create_expense_objects(expenses, workspace_id=workspace_id) | |
| expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) | |
| assert len(expense_objects) == 3, f'Expected 3 expenses, got {len(expense_objects)}' | |
| # Test for SINGLE_LINE_ITEM split expense grouping | |
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | |
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) | |
| assert len(groups) == 3, f'Expected 3 groups, got {len(groups)}' | |
| old_count = len(groups) | |
| # Test for MULTIPLE_LINE_ITEM split expense grouping | |
| expense_group_settings.split_expense_grouping = 'MULTIPLE_LINE_ITEM' | |
| expense_group_settings.save() | |
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | |
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]).distinct() | |
| assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}' | |
| def test_split_expense_grouping_with_same_and_different_ids(db, update_config_for_split_expense_grouping): | |
| ''' | |
| Test for grouping of 2 expenses with the same bank transaction id, | |
| and one expense with a different bank transaction id | |
| ''' | |
| workspace_id = 1 | |
| # Update settings | |
| configuration = Configuration.objects.get(workspace_id=workspace_id) | |
| expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) | |
| update_config_for_split_expense_grouping(configuration, expense_group_settings) | |
| # Get reference to expense objects | |
| expenses = data['ccc_split_expenses'][:3] | |
| expenses[0]['bank_transaction_id'] = 'sample_1' | |
| expenses[1]['bank_transaction_id'] = 'sample_1' | |
| expenses[2]['bank_transaction_id'] = 'sample_2' | |
| Expense.create_expense_objects(expenses, workspace_id=workspace_id) | |
| expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) | |
| assert len(expense_objects) == 3, f'Expected 3 expenses, got {len(expense_objects)}' | |
| # Test for SINGLE_LINE_ITEM split expense grouping | |
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | |
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) | |
| assert len(groups) == 3, f'Expected 3 groups, got {len(groups)}' | |
| old_count = len(groups) | |
| # Test for MULTIPLE_LINE_ITEM split expense grouping | |
| expense_group_settings.split_expense_grouping = 'MULTIPLE_LINE_ITEM' | |
| expense_group_settings.save() | |
| ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) | |
| groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]).distinct() | |
| new_groups = groups[old_count:] | |
| assert len(new_groups) == 2, 'Expected 2 new groups to be created' | |
| # Verify group contents | |
| for group in new_groups: | |
| expenses_in_group = group.expenses.all() | |
| if len(expenses_in_group) == 2: | |
| # Group with same bank_transaction_id | |
| assert all(exp.bank_transaction_id == 'sample_1' for exp in expenses_in_group), \ | |
| 'Expenses with same bank_transaction_id should be grouped together' | |
| else: | |
| # Single expense group | |
| assert expenses_in_group[0].bank_transaction_id == 'sample_2', \ | |
| 'Single expense should have different bank_transaction_id' |
tests/test_netsuite/conftest.py
Outdated
| for expense in expense_group.expenses.all(): | ||
| expense.workspace_id = 1 | ||
| expense.save() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate workspace ID update logic
The workspace ID update logic is duplicated across multiple fixtures. Consider:
- Creating a helper function to update expense workspace IDs
- Documenting why specific workspace IDs are required
Example refactor:
def _update_expense_workspace_ids(expense_group, workspace_id):
"""
Update workspace IDs for all expenses in an expense group.
Args:
expense_group (ExpenseGroup): The expense group containing expenses
workspace_id (int): The workspace ID to set
"""
for expense in expense_group.expenses.all():
expense.workspace_id = workspace_id
expense.save()Then use it in fixtures:
_update_expense_workspace_ids(expense_group, workspace_id=1)Also applies to: 95-97, 252-254, 266-268
| def test_construct_expense_report_with_tax_balancing(create_expense_report, add_tax_destination_attributes): | ||
| netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=1) | ||
| netsuite_connection = NetSuiteConnector(netsuite_credentials=netsuite_credentials, workspace_id=1) | ||
| general_mapping = GeneralMapping.objects.get(workspace_id=1) | ||
|
|
||
| # without tax balancing | ||
| expense_report, expense_report_lineitem = create_expense_report | ||
| expense_report_lineitem[0].amount = 100 | ||
| expense_report_lineitem[0].tax_amount = 3 | ||
| expense_report_lineitem[0].tax_item_id = '103578' | ||
|
|
||
| expense_report_object = netsuite_connection._NetSuiteConnector__construct_expense_report(expense_report, expense_report_lineitem, general_mapping) | ||
|
|
||
| assert len(expense_report_object['expenseList']) == 1 | ||
| assert expense_report_object['expenseList'][0]['amount'] == 97 | ||
| assert expense_report_object['expenseList'][0]['taxCode']['internalId'] == '103578' | ||
| assert expense_report_object['expenseList'][0]['tax1Amt'] == 3 | ||
|
|
||
| # with tax balancing | ||
| general_mapping.is_tax_balancing_enabled = True | ||
| general_mapping.save() | ||
|
|
||
| expense_report_object = netsuite_connection._NetSuiteConnector__construct_expense_report(expense_report, expense_report_lineitem, general_mapping) | ||
|
|
||
| assert len(expense_report_object['expenseList']) == 2 | ||
| assert expense_report_object['expenseList'][0]['amount'] == 60 | ||
| assert expense_report_object['expenseList'][0]['taxCode']['internalId'] == '103578' | ||
| assert expense_report_object['expenseList'][0]['tax1Amt'] == 3 | ||
| assert expense_report_object['expenseList'][1]['amount'] == 37 | ||
| assert expense_report_object['expenseList'][1]['taxCode']['internalId'] == general_mapping.default_tax_code_id | ||
|
|
||
|
|
||
| # with tax balancing enabled and right tax amount | ||
| expense_report_lineitem[0].amount = 100 | ||
| expense_report_lineitem[0].tax_amount = 4.76 | ||
| expense_report_lineitem[0].tax_item_id = '103578' | ||
|
|
||
| expense_report_object = netsuite_connection._NetSuiteConnector__construct_expense_report(expense_report, expense_report_lineitem, general_mapping) | ||
|
|
||
| assert len(expense_report_object['expenseList']) == 1 | ||
| assert expense_report_object['expenseList'][0]['amount'] == 95.24 | ||
| assert expense_report_object['expenseList'][0]['taxCode']['internalId'] == '103578' | ||
| assert expense_report_object['expenseList'][0]['tax1Amt'] == 4.76 | ||
|
|
||
| general_mapping.is_tax_balancing_enabled = False | ||
| general_mapping.save() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure test isolation when modifying shared objects
In test_construct_expense_report_with_tax_balancing, the general_mapping.is_tax_balancing_enabled attribute is modified and saved. This could affect other tests if general_mapping is a shared object. Consider using a fixture or mocking general_mapping to prevent side effects between tests.
apps/netsuite/connector.py
Outdated
| def construct_bill_lineitems( | ||
| self, | ||
| bill_lineitems: List[BillLineitem], | ||
| attachment_links: Dict, | ||
| cluster_domain: str, org_id: str, | ||
| override_tax_details: bool, | ||
| general_mapping: GeneralMapping | ||
| ) -> List[Dict]: |
There was a problem hiding this comment.
Update All Calls to construct_bill_lineitems with New Parameter
The method signature of construct_bill_lineitems now includes the general_mapping parameter. Ensure that all calls to this method throughout the codebase are updated to pass this new parameter to prevent TypeError exceptions.
Run the following script to find all usages of construct_bill_lineitems that may need updating:
#!/bin/bash
# Description: Find all calls to `construct_bill_lineitems` missing the new `general_mapping` parameter.
rg "construct_bill_lineitems\((?!.*general_mapping)" -A 0 apps/netsuite/
apps/netsuite/connector.py
Outdated
| if general_mapping.is_tax_balancing_enabled and round(line.tax_amount, 2) != expected_tax_amount: | ||
| # Recalculate the net amount based on the modified tax | ||
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) |
There was a problem hiding this comment.
Handle Potential Division by Zero in handle_taxed_line_items
At line 1187, the calculation recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) could result in a division by zero if tax_item_rate is zero. This would raise a ZeroDivisionError at runtime. Ensure to check if tax_item_rate is not zero before performing the division.
Apply this diff to handle the division by zero:
1185 if general_mapping.is_tax_balancing_enabled and round(line.tax_amount, 2) != expected_tax_amount:
+ if tax_item_rate != 0:
# Recalculate the net amount based on the modified tax
recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2)
+ else:
+ recalculated_net_amount = 0📝 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 general_mapping.is_tax_balancing_enabled and round(line.tax_amount, 2) != expected_tax_amount: | |
| # Recalculate the net amount based on the modified tax | |
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) | |
| if general_mapping.is_tax_balancing_enabled and round(line.tax_amount, 2) != expected_tax_amount: | |
| if tax_item_rate != 0: | |
| # Recalculate the net amount based on the modified tax | |
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) | |
| else: | |
| recalculated_net_amount = 0 |
apps/netsuite/connector.py
Outdated
| @@ -1351,7 +1450,7 @@ def __construct_bill(self, bill: Bill, bill_lineitems: List[BillLineitem]) -> Di | |||
| org_id = Workspace.objects.get(id=bill.expense_group.workspace_id).fyle_org_id | |||
|
|
|||
| tax_details_list = None | |||
| expense_list, item_list = self.construct_bill_lineitems(bill_lineitems, {}, cluster_domain, org_id, bill.override_tax_details) | |||
| expense_list, item_list = self.construct_bill_lineitems(bill_lineitems, {}, cluster_domain, org_id, bill.override_tax_details, general_mappings) | |||
There was a problem hiding this comment.
Assign Variables Returned from construct_bill_lineitems Correctly
At line 1453, expense_list, item_list are assigned from construct_bill_lineitems. Ensure that the method returns two values as expected. If construct_bill_lineitems is modified to return a single value or a different structure, update the assignment accordingly.
Apply this diff if construct_bill_lineitems returns a single list:
- expense_list, item_list = self.construct_bill_lineitems(...)
+ line_items = self.construct_bill_lineitems(...)Committable suggestion skipped: line range outside the PR's diff.
Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-script
branch
from
4c13fd2 to
716764e
Compare
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-core-logic
branch
from
b9ff9c0 to
4103aaf
Compare
* feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
JustARatherRidiculouslyLongUsername
merged commit 5348896
into
split-expenses-core-logic
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/test_fyle/conftest.py (1)
31-47: LGTM with minor suggestions for the new fixture.The fixture implementation follows pytest best practices and correctly sets up the configuration for split expense grouping tests. However, consider adding docstring documentation to clarify the fixture's purpose and parameters.
Add documentation to improve maintainability:
@pytest.fixture def update_config_for_split_expense_grouping(db): + """Configure expense group settings for split expense grouping tests. + + Args: + general_settings: General settings object to be updated + expense_group_settings: Expense group settings object to be updated + """ def _update_config_for_split_expense_grouping(general_settings, expense_group_settings):tests/test_fyle/test_models.py (1)
170-207: Improve test maintainability and readabilityThe test structure could be improved for better maintainability and clarity:
- Extract test data setup into helper methods
- Use constants for test values
- Add group content verification
+# Constants at module level +BANK_TRANSACTION_ID_1 = 'sample_1' +BANK_TRANSACTION_ID_2 = 'sample_2' + +def setup_test_expenses(base_expenses, transaction_ids): + """Helper to setup test expenses with given transaction IDs""" + expenses = base_expenses.copy() + for expense, tx_id in zip(expenses, transaction_ids): + expense['bank_transaction_id'] = tx_id + return expenses + def test_split_expense_grouping_with_same_and_different_ids(db, update_config_for_split_expense_grouping): ''' - Test for grouping of 2 expenses with the same bank transaction id, - and one expense with a different bank transaction id + Test expense grouping behavior with mixed bank transaction IDs + + Scenario: + - 2 expenses sharing bank_transaction_id_1 + - 1 expense with bank_transaction_id_2 + + Expected outcomes: + - SINGLE_LINE_ITEM: Creates individual groups for each expense + - MULTIPLE_LINE_ITEM: Groups expenses by bank_transaction_id ''' workspace_id = 1 + TEST_EXPENSE_COUNT = 3 # Update settings configuration = Configuration.objects.get(workspace_id=workspace_id) expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) update_config_for_split_expense_grouping(configuration, expense_group_settings) - # Get reference to expense objects - expenses = data['ccc_split_expenses'][:3] - expenses[0]['bank_transaction_id'] = 'sample_1' - expenses[1]['bank_transaction_id'] = 'sample_1' - expenses[2]['bank_transaction_id'] = 'sample_2' + # Setup test data + expenses = setup_test_expenses( + data['ccc_split_expenses'][:TEST_EXPENSE_COUNT], + [BANK_TRANSACTION_ID_1, BANK_TRANSACTION_ID_1, BANK_TRANSACTION_ID_2] + ) Expense.create_expense_objects(expenses, workspace_id=workspace_id) expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) - assert len(expense_objects) == 3, f'Expected 3 expenses, got {len(expense_objects)}' + assert len(expense_objects) == TEST_EXPENSE_COUNT, \ + f'Expected {TEST_EXPENSE_COUNT} expenses, got {len(expense_objects)}' # Test for SINGLE_LINE_ITEM split expense grouping ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) - assert len(groups) == 3, f'Expected 3 groups, got {len(groups)}' + assert len(groups) == TEST_EXPENSE_COUNT, \ + f'Expected {TEST_EXPENSE_COUNT} groups, got {len(groups)}' + + # Verify each expense is in its own group + for group in groups: + assert group.expenses.count() == 1, 'Each expense should be in its own group' old_count = len(groups) # Test for MULTIPLE_LINE_ITEM split expense grouping @@ -203,6 +203,15 @@ ExpenseGroup.create_expense_groups_by_report_id_fund_source(expense_objects, configuration, workspace_id) groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]).distinct() - assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}' + new_groups = groups[old_count:] + expected_new_group_count = 2 # One group for sample_1 (2 expenses) and one for sample_2 (1 expense) + assert len(new_groups) == expected_new_group_count, \ + f'Expected {expected_new_group_count} new groups, got {len(new_groups)}' + + # Verify expenses are grouped by bank_transaction_id + for group in new_groups: + expenses_in_group = group.expenses.all() + assert all(exp.bank_transaction_id == expenses_in_group[0].bank_transaction_id + for exp in expenses_in_group), 'All expenses in group should share same bank_transaction_id'apps/fyle/models.py (2)
215-217: Consider adding docstring to the default function.While the function is simple, it would be helpful to document why 'MULTIPLE_LINE_ITEM' is chosen as the default value.
def get_default_split_expense_grouping(): + """ + Returns the default split expense grouping setting. + Default is MULTIPLE_LINE_ITEM to support granular expense tracking. + """ return 'MULTIPLE_LINE_ITEM'
421-458: Review the complexity of expense grouping logic.The corporate credit card expense grouping logic has become quite complex with nested conditions. Consider extracting this into a separate method for better maintainability.
def create_expense_groups_by_report_id_fund_source(expense_objects: List[Expense], configuration: Configuration, workspace_id): + def _group_ccc_expenses(corporate_credit_card_expenses, expense_group_settings, corporate_credit_card_expense_group_field): + """ + Groups corporate credit card expenses based on configuration settings + """ + if ( + configuration.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' and + expense_group_settings.split_expense_grouping == 'MULTIPLE_LINE_ITEM' + ): + ccc_expenses_without_bank_transaction_id = list( + filter(lambda expense: not expense.bank_transaction_id, corporate_credit_card_expenses) + ) + ccc_expenses_with_bank_transaction_id = list( + filter(lambda expense: expense.bank_transaction_id, corporate_credit_card_expenses) + ) + + expense_groups = [] + if ccc_expenses_without_bank_transaction_id: + groups_without_bank_transaction_id = _group_expenses( + ccc_expenses_without_bank_transaction_id, + corporate_credit_card_expense_group_field, + workspace_id + ) + expense_groups.extend(groups_without_bank_transaction_id) + + if ccc_expenses_with_bank_transaction_id: + split_expense_group_fields = [ + field for field in corporate_credit_card_expense_group_field + if field not in ('expense_id', 'expense_number') + ] + split_expense_group_fields.append('bank_transaction_id') + + groups_with_bank_transaction_id = _group_expenses( + ccc_expenses_with_bank_transaction_id, + split_expense_group_fields, + workspace_id + ) + expense_groups.extend(groups_with_bank_transaction_id) + + return expense_groups + + return _group_expenses( + corporate_credit_card_expenses, + corporate_credit_card_expense_group_field, + workspace_id + )
🛑 Comments failed to post (1)
tests/test_fyle/test_models.py (1)
209-245: 🛠️ Refactor suggestion
Reduce code duplication using parameterized tests
The three test functions share similar structure and could be consolidated using pytest's parameterize feature to improve maintainability and reduce duplication.
Consider refactoring all three test functions into a single parameterized test:
+import pytest + +@pytest.mark.parametrize('test_scenario', [ + { + 'name': 'no_bank_transaction_id', + 'transaction_ids': [None, None], + 'expected_single_line_groups': 2, + 'expected_multiple_line_groups': 2, + 'description': 'Expenses with no bank transaction ID should always be in separate groups' + }, + { + 'name': 'mixed_transaction_ids', + 'transaction_ids': [BANK_TRANSACTION_ID_1, BANK_TRANSACTION_ID_1, BANK_TRANSACTION_ID_2], + 'expected_single_line_groups': 3, + 'expected_multiple_line_groups': 2, + 'description': 'Expenses with same transaction ID should be grouped in MULTIPLE_LINE_ITEM mode' + }, + { + 'name': 'paired_transaction_ids', + 'transaction_ids': [ + BANK_TRANSACTION_ID_1, BANK_TRANSACTION_ID_1, + BANK_TRANSACTION_ID_2, BANK_TRANSACTION_ID_2 + ], + 'expected_single_line_groups': 4, + 'expected_multiple_line_groups': 2, + 'description': 'Each pair of expenses should be grouped in MULTIPLE_LINE_ITEM mode' + } +]) +def test_split_expense_grouping(db, update_config_for_split_expense_grouping, test_scenario): + ''' + Test expense grouping behavior with different bank transaction ID scenarios + + For each scenario, verifies: + 1. SINGLE_LINE_ITEM mode creates individual groups + 2. MULTIPLE_LINE_ITEM mode groups by bank_transaction_id + ''' + workspace_id = 1 + + # Update settings + configuration = Configuration.objects.get(workspace_id=workspace_id) + expense_group_settings = ExpenseGroupSettings.objects.get(workspace_id=workspace_id) + update_config_for_split_expense_grouping(configuration, expense_group_settings) + + # Setup test data + expenses = setup_test_expenses( + data['ccc_split_expenses'][:len(test_scenario['transaction_ids'])], + test_scenario['transaction_ids'] + ) + + Expense.create_expense_objects(expenses, workspace_id=workspace_id) + expense_objects = Expense.objects.filter( + expense_id__in=[expense['id'] for expense in expenses] + ) + + # Test SINGLE_LINE_ITEM mode + ExpenseGroup.create_expense_groups_by_report_id_fund_source( + expense_objects, configuration, workspace_id + ) + groups = ExpenseGroup.objects.filter( + expenses__expense_id__in=[expense['id'] for expense in expenses] + ) + assert len(groups) == test_scenario['expected_single_line_groups'], \ + f"Expected {test_scenario['expected_single_line_groups']} groups in SINGLE_LINE_ITEM mode" + + # Verify each expense is in its own group + for group in groups: + assert group.expenses.count() == 1, 'Each expense should be in its own group' + old_count = len(groups) + + # Test MULTIPLE_LINE_ITEM mode + expense_group_settings.split_expense_grouping = 'MULTIPLE_LINE_ITEM' + expense_group_settings.save() + + ExpenseGroup.create_expense_groups_by_report_id_fund_source( + expense_objects, configuration, workspace_id + ) + groups = ExpenseGroup.objects.filter( + expenses__expense_id__in=[expense['id'] for expense in expenses] + ).distinct() + + new_groups = groups[old_count:] + assert len(new_groups) == test_scenario['expected_multiple_line_groups'], \ + f"Expected {test_scenario['expected_multiple_line_groups']} groups in MULTIPLE_LINE_ITEM mode" + + # Verify expenses are grouped by bank_transaction_id + for group in new_groups: + expenses_in_group = group.expenses.all() + if expenses_in_group[0].bank_transaction_id is not None: + assert all( + exp.bank_transaction_id == expenses_in_group[0].bank_transaction_id + for exp in expenses_in_group + ), 'All expenses in group should share same bank_transaction_id' + else: + assert expenses_in_group.count() == 1, \ + 'Expenses with no bank_transaction_id should be in separate groups'This refactoring:
- Reduces code duplication
- Makes test scenarios more maintainable
- Improves readability with clear scenario descriptions
- Makes it easier to add new test scenarios
Committable suggestion skipped: line range outside the PR's diff.
* feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
…ings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
…680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop (cherry picked from commit c0309e8)
Clickup
https://app.clickup.com/t/86cx0vkdr
Summary by CodeRabbit