feat: support split expense grouping #681
Conversation
) * 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
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces significant changes to the Fyle application and its related components, primarily focusing on enhancing expense management features. Key modifications include the addition of new fields such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@JustARatherRidiculouslyLongUsername has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 13
🧹 Outside diff range and nitpick comments (13)
tests/test_fyle/fixtures.py (1)
669-827: Consider adding edge cases to test dataThe current test data covers the basic case of grouping split expenses with:
- Different amounts (1,2,3,4)
- Same vendor, project, and timestamps
- Same bank transaction ID
Consider adding test cases for edge scenarios:
- Different currencies in split expenses
- Different cost centers or custom fields
- Split expenses with tax amounts
- Expenses with attachments (non-empty file_ids)
tests/test_fyle/conftest.py (1)
33-47: Consider making the fixture more flexible.The fixture uses hardcoded values which might limit its reusability in different test scenarios. Consider:
- Making the configuration values parameterizable
- Adding error handling for the save operations
Here's a suggested improvement:
@pytest.fixture def update_config_for_split_expense_grouping(db): - def _update_config_for_split_expense_grouping(general_settings, expense_group_settings): + def _update_config_for_split_expense_grouping( + general_settings, + expense_group_settings, + expenses_object='CREDIT CARD CHARGE', + grouping_type='SINGLE_LINE_ITEM', + expense_fields=None + ): + if expense_fields is None: + expense_fields = [ + 'expense_id', + 'claim_number', + 'fund_source', + 'employee_email', + 'report_id', + 'spent_at' + ] + try: - general_settings.corporate_credit_card_expenses_object = 'CREDIT CARD CHARGE' + general_settings.corporate_credit_card_expenses_object = expenses_object general_settings.save() - expense_group_settings.split_expense_grouping = 'SINGLE_LINE_ITEM' + expense_group_settings.split_expense_grouping = grouping_type - 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_fields expense_group_settings.save() + except Exception as e: + pytest.fail(f"Failed to update settings: {str(e)}") return _update_config_for_split_expense_groupingapps/workspaces/apis/export_settings/serializers.py (1)
Line range hint
88-99: Consider API versioning for breaking changes.Adding a required field is a breaking change that might affect API clients. Consider:
- API versioning strategy (e.g., /v2/export-settings)
- Gradual rollout strategy with feature flags
- Documentation updates for API consumers
tests/test_fyle/test_models.py (2)
134-168: Enhance test robustness and maintainabilityWhile the test logic is sound, consider these improvements:
- Use more descriptive assertion messages that explain the expected behavior
- Add cleanup of created objects using pytest fixtures
- Consider parameterizing the workspace_id instead of hardcoding
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: + - In SINGLE_LINE_ITEM mode: Should create separate groups + - In MULTIPLE_LINE_ITEM mode: Should create separate groups ''' - workspace_id = 1 + workspace_id = settings.TEST_WORKSPACE_ID # Consider using a settings variable # 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] 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, 'Should create exactly 2 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) == 2, f'Expected 2 groups, got {len(groups)}' + assert len(groups) == 2, 'SINGLE_LINE_ITEM: Should create separate groups for expenses without bank_transaction_id' 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}' + assert len(groups) - old_count == 2, 'MULTIPLE_LINE_ITEM: Should create separate groups for expenses without bank_transaction_id'
134-245: Consider organizing split expense grouping tests into a classThe three split expense grouping tests are related and would benefit from being organized into a test class. This would:
- Improve test organization and readability
- Allow sharing of common setup code
- Make it easier to add more test cases in the future
@pytest.mark.django_db class TestSplitExpenseGrouping: @pytest.fixture(autouse=True) def setup(self, update_config_for_split_expense_grouping): self.workspace_id = 1 self.configuration = Configuration.objects.get(workspace_id=self.workspace_id) self.expense_group_settings = ExpenseGroupSettings.objects.get( workspace_id=self.workspace_id ) update_config_for_split_expense_grouping( self.configuration, self.expense_group_settings ) def setup_expenses(self, expenses, bank_transaction_ids): for expense, bank_id in zip(expenses, bank_transaction_ids): expense['bank_transaction_id'] = bank_id Expense.create_expense_objects(expenses, workspace_id=self.workspace_id) return Expense.objects.filter( expense_id__in=[expense['id'] for expense in expenses] ) def test_no_bank_transaction_id(self): # Test implementation... def test_same_and_different_ids(self): # Test implementation... def test_pairs_of_same_ids(self): # Test implementation...tests/test_netsuite/test_models.py (3)
367-372: Fix boolean comparison and improve test assertionsThe boolean comparison can be improved, and the test assertions could be more comprehensive.
- line = credit_card_charge_lineitems[0] - assert line.amount == 100.00 - assert line.memo == '[email protected] - Accounts Payable - 2021-11-15 - C/2021/11/R/6 - ' - assert line.billable == False + assert credit_card_charge_lineitems, "Expected non-empty line items" + line = credit_card_charge_lineitems[0] + assert line.amount == 100.00, "Unexpected amount" + assert line.memo == '[email protected] - Accounts Payable - 2021-11-15 - C/2021/11/R/6 - ', "Unexpected memo format" + assert not line.billable, "Expected non-billable line item"🧰 Tools
🪛 Ruff (0.7.0)
367-367:
CreditCardChargeLineItemmay be undefined, or defined from star imports(F405)
372-372: Avoid equality comparisons to
False; useif not line.billable:for false checksReplace with
not line.billable(E712)
Line range hint
342-372: Add test cases for split expense grouping featureThe test suite is missing coverage for the new split expense grouping functionality. Consider adding the following test cases:
- Verify the
split_expense_groupingtoggle behavior- Test grouping of split expenses into a single Credit Card Charge
- Validate the correct handling of multiple line items within a grouped charge
Here's a suggested test case structure:
def test_split_expense_grouping(): # Test with split_expense_grouping enabled configuration = Configuration.objects.get(workspace_id=2) configuration.split_expense_grouping = True configuration.save() # Create test data with split expenses expense_group = create_expense_group_with_split_expenses() # Verify grouping behavior credit_card = CreditCardCharge.create_credit_card_charge(expense_group) credit_card_charge_lineitems = CreditCardChargeLineItem.create_credit_card_charge_lineitems( expense_group, configuration ) # Assert grouped expenses assert len(credit_card_charge_lineitems) > 1, "Expected multiple line items for split expense" assert all(item.credit_card_charge_id == credit_card.id for item in credit_card_charge_lineitems) # Test with split_expense_grouping disabled configuration.split_expense_grouping = False configuration.save() # Verify default behavior credit_card = CreditCardCharge.create_credit_card_charge(expense_group) credit_card_charge_lineitems = CreditCardChargeLineItem.create_credit_card_charge_lineitems( expense_group, configuration ) # Assert ungrouped expenses assert len(credit_card_charge_lineitems) == 1, "Expected single line item when grouping is disabled"🧰 Tools
🪛 Ruff (0.7.0)
341-341:
CreditCardChargemay be undefined, or defined from star imports(F405)
343-343:
CreditCardChargeLineItemmay be undefined, or defined from star imports(F405)
348-348: Avoid equality comparisons to
False; useif not line.billable:for false checksReplace with
not line.billable(E712)
Line range hint
1-7: Replace star imports with explicit importsThe use of star imports (
from module import *) makes it unclear where symbols likeCreditCardChargeLineItemare defined and can lead to naming conflicts. Consider explicitly importing the required symbols.-from apps.netsuite.models import * -from apps.netsuite.tasks import * -from apps.netsuite.queue import * +from apps.netsuite.models import ( + Bill, BillLineitem, CreditCardCharge, + CreditCardChargeLineItem, ExpenseReport, + ExpenseReportLineItem +) +from apps.netsuite.tasks import create_bill, load_attachments +from apps.netsuite.queue import ( + get_department_id_or_none, get_class_id_or_none, + get_location_id_or_none, get_custom_segments +)🧰 Tools
🪛 Ruff (0.7.0)
341-341:
CreditCardChargemay be undefined, or defined from star imports(F405)
343-343:
CreditCardChargeLineItemmay be undefined, or defined from star imports(F405)
348-348: Avoid equality comparisons to
False; useif not line.billable:for false checksReplace with
not line.billable(E712)
tests/test_netsuite/test_connector.py (2)
307-314: Consider adding array bounds check before accessing line items.While the test setup is correct, it would be more robust to verify that the line items array is not empty before accessing the first element.
credit_card_charge, credit_card_charge_lineitems = create_credit_card_charge +if not credit_card_charge_lineitems: + pytest.fail("No line items available for testing") + item = credit_card_charge_lineitems[0] item.amount = 100 item.tax_amount = 3 item.tax_item_id = '103578'
333-337: LGTM! Consider documenting the significance of these test values.The test correctly sets up a specific tax balancing scenario. It would be helpful to add a comment explaining why these specific values (100, 4.76) were chosen and what edge case they're testing.
+# Testing tax balancing with a 4.76% tax rate scenario item.amount = 100 item.tax_amount = 4.76 item.tax_item_id = '103578'apps/netsuite/models.py (2)
722-734: Simplify nested conditions in department handling.Combine nested if statements into single conditions for better readability.
- if expense_group.fund_source == 'CCC' and general_mappings.use_employee_department and \ - general_mappings.department_level in ('ALL', 'TRANSACTION_LINE'): - employee_mapping = EmployeeMapping.objects.filter( - source_employee__value=expense_group.description.get('employee_email'), - workspace_id=expense_group.workspace_id - ).first() - if employee_mapping and employee_mapping.destination_employee: - if employee_mapping.destination_employee.detail.get('department_id'): - department_id = employee_mapping.destination_employee.detail.get('department_id') + if (expense_group.fund_source == 'CCC' and + general_mappings.use_employee_department and + general_mappings.department_level in ('ALL', 'TRANSACTION_LINE') and + employee_mapping and + employee_mapping.destination_employee and + employee_mapping.destination_employee.detail.get('department_id')): + department_id = employee_mapping.destination_employee.detail.get('department_id')🧰 Tools
🪛 Ruff (0.7.0)
728-729: Use a single
ifstatement instead of nestedifstatements(SIM102)
732-733: Use a single
ifstatement instead of nestedifstatements(SIM102)
736-749: Simplify nested conditions in location handling.Similar to department handling, combine nested if statements for better readability.
- if not location_id: - if general_mappings.location_id and general_mappings.location_level in ['TRANSACTION_LINE', 'ALL']: - location_id = general_mappings.location_id + if not location_id and general_mappings.location_id and general_mappings.location_level in ['TRANSACTION_LINE', 'ALL']: + location_id = general_mappings.location_id🧰 Tools
🪛 Ruff (0.7.0)
747-748: Use a single
ifstatement instead of nestedifstatements(SIM102)
apps/netsuite/connector.py (1)
1584-1584: Remove Unused Parametercluster_domainfrom Method SignatureThe parameter
cluster_domainin theconstruct_credit_card_charge_lineitemsmethod is not used within the method body. Removing it will simplify the method signature and improve readability.Apply this diff:
-def construct_credit_card_charge_lineitems( - self, credit_card_charge_lineitems: List[CreditCardChargeLineItem], general_mapping: GeneralMapping, - attachment_links: Dict, cluster_domain: str, org_id: str) -> List[Dict]: +def construct_credit_card_charge_lineitems( + self, credit_card_charge_lineitems: List[CreditCardChargeLineItem], general_mapping: GeneralMapping, + attachment_links: Dict, org_id: str) -> List[Dict]:Ensure to update all calls to this method accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
apps/fyle/migrations/0035_support_split_expense_grouping.py(1 hunks)apps/fyle/models.py(10 hunks)apps/netsuite/connector.py(3 hunks)apps/netsuite/models.py(1 hunks)apps/netsuite/tasks.py(3 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(7 hunks)tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/fixtures.py(10 hunks)tests/test_fyle/test_models.py(2 hunks)tests/test_netsuite/conftest.py(1 hunks)tests/test_netsuite/test_connector.py(4 hunks)tests/test_netsuite/test_models.py(2 hunks)tests/test_workspaces/test_apis/test_export_settings/fixtures.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/sql/scripts/030-default-split-expense-grouping-for-old-orgs.sql
🧰 Additional context used
🪛 Ruff (0.7.0)
apps/netsuite/models.py
700-700: Comparison to None should be cond is None
Replace with cond is None
(E711)
728-729: Use a single if statement instead of nested if statements
(SIM102)
732-733: Use a single if statement instead of nested if statements
(SIM102)
747-748: Use a single if statement instead of nested if statements
(SIM102)
tests/test_netsuite/test_models.py
343-343: CreditCardChargeLineItem may be undefined, or defined from star imports
(F405)
348-348: Avoid equality comparisons to False; use if not line.billable: for false checks
Replace with not line.billable
(E712)
367-367: CreditCardChargeLineItem may be undefined, or defined from star imports
(F405)
372-372: Avoid equality comparisons to False; use if not line.billable: for false checks
Replace with not line.billable
(E712)
🔇 Additional comments (29)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
11909-11909: LGTM!
The sequence value update to 205 aligns with the new migration for split expense grouping support.
5-5: Verify PostgreSQL version compatibility
The database version has been downgraded to 15.7 while the dump version remains at 15.9. This version mismatch might cause compatibility issues during database restoration.
✅ Verification successful
PostgreSQL version downgrade is safe to proceed
The version change from 15.9 to 15.7 is a minor version downgrade within the same major version (15.x). Based on the evidence:
- The project uses PostgreSQL 15 in the Dockerfile
- The version difference (15.9 to 15.7) only involves bug fixes and security updates
- No breaking changes or schema incompatibilities exist between these patch versions
- The project primarily uses standard PostgreSQL features (JSONField, ArrayField) that are stable across 15.x versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any breaking changes between PostgreSQL 15.7 and 15.9
# Check if README or documentation mentions PostgreSQL version requirements
fd -t f "README|CHANGELOG" -X grep -l "postgres"
# Look for any PostgreSQL version constraints in project files
rg "postgresql|postgres" -g "!*.sql"
Length of output: 6814
962-963:
Consider adding a default value for split_expense_grouping
The new column split_expense_grouping is marked as NOT NULL but doesn't have a default value. This might cause issues when running migrations on existing data.
Consider modifying the column definition to include a default value:
- split_expense_grouping character varying(100) NOT NULL
+ split_expense_grouping character varying(100) NOT NULL DEFAULT 'SINGLE_LINE_ITEM'Likely invalid or redundant comment.
1153-1154: Verify bank_transaction_id nullability
The bank_transaction_id column is nullable, which aligns with the PR objectives as not all expenses will have bank transactions. However, ensure that the application code properly handles null values.
✅ Verification successful
Let me generate new shell scripts to search for bank_transaction_id handling in the codebase, focusing on Python files and using more specific patterns:
The previous search was too broad and included many SQL files. Let's focus on the Python code implementation to verify how null bank_transaction_id values are handled.
Let me try one more focused search to find the bank_transaction_id handling in the Python code:
This should help us see how bank_transaction_id is used in the expense model and grouping logic.
Based on the code analysis, I can now provide a final response:
Bank transaction ID handling is properly implemented
The codebase handles null bank_transaction_id values appropriately:
- The field is correctly defined as nullable in the Expense model:
bank_transaction_id = models.CharField(max_length=255, null=True, blank=True) - The expense grouping logic explicitly handles both cases:
- Expenses without bank_transaction_id are grouped separately using
ccc_expenses_without_bank_transaction_id - Expenses with bank_transaction_id are grouped using
ccc_expenses_with_bank_transaction_id
- Expenses without bank_transaction_id are grouped separately using
- The filtering is done safely using Python's truthiness check:
filter(lambda expense: expense.bank_transaction_id, ...)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the application code handles null bank_transaction_id
# Look for bank_transaction_id usage in the codebase
rg "bank_transaction_id" --type py -B 2 -A 2
# Look for null checks on bank_transaction_id
ast-grep --pattern 'if $_.bank_transaction_id'
Length of output: 164369
Script:
#!/bin/bash
# Look for bank_transaction_id usage in expense grouping logic
ast-grep --pattern 'if $_.bank_transaction_id:
$$$'
# Look for bank_transaction_id null checks
ast-grep --pattern 'if not $_.bank_transaction_id:
$$$'
# Look for bank_transaction_id in expense model and related code
rg "bank_transaction_id" -C 5 --type py apps/fyle/models.py
Length of output: 102118
Script:
#!/bin/bash
# Look for bank_transaction_id handling in expense grouping logic with more context
rg "bank_transaction_id" -C 10 apps/fyle/models.py
# Look for expense model definition
rg "class Expense\(" -A 10 apps/fyle/models.py
Length of output: 7245
tests/test_fyle/fixtures.py (3)
1-1: LGTM: Import required for datetime test data
The datetime import is correctly placed and necessary for creating proper timestamp objects in the test fixtures.
1016-1017: Verify test coverage for split expense grouping modes
The fixtures set split_expense_grouping: "MULTIPLE_LINE_ITEM". To ensure comprehensive testing:
Also applies to: 1121-1122
✅ Verification successful
Based on the search results, I can now generate a final response regarding the test coverage for split expense grouping modes:
Test coverage for split expense grouping modes is comprehensive
The codebase has thorough test coverage for both supported grouping modes:
- Tests verify
SINGLE_LINE_ITEMandMULTIPLE_LINE_ITEMmodes in multiple scenarios:- Expenses with no bank transaction ID (test_split_expense_grouping_with_no_bank_transaction_id)
- Expenses with same and different transaction IDs (test_split_expense_grouping_with_same_and_different_ids)
- Multiple pairs of expenses with same transaction IDs (test_split_expense_grouping_pairs_of_same_ids)
Each test validates the grouping behavior by checking the resulting expense groups count and composition for both modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if tests cover all possible grouping modes
echo "Searching for split_expense_grouping enum/choices definition..."
rg -A 5 'split_expense_grouping.*choices'
echo "Searching for test cases covering different modes..."
rg -g '*/test_*.py' 'split_expense_grouping.*' tests/
Length of output: 351
Script:
#!/bin/bash
# Let's try a broader search for split_expense_grouping to understand its usage and possible values
echo "Searching for split_expense_grouping in all Python files..."
rg "split_expense_grouping" -t py
echo -e "\nSearching for any enum or constant definitions that might contain grouping modes..."
rg -B2 -A2 "MULTIPLE_LINE_ITEM" -t py
echo -e "\nSearching test files specifically for expense grouping test cases..."
rg -g "test_*.py" "(expense.*group|group.*expense)" -t py
Length of output: 63476
249-249: Verify bank transaction ID pattern
All test expenses use the same bank_transaction_id: 'btxnqmATEEBkJ4'. While this helps test the grouping functionality, we should verify if this matches the actual bank transaction ID format from the payment processor.
Also applies to: 291-291, 333-333, 622-622, 665-665, 673-673, 713-713, 753-753, 793-793, 872-872, 915-915
✅ Verification successful
Let me try a different search pattern to find more information about bank transaction IDs.
From the search results, I found the model definition in apps/fyle/models.py that shows bank_transaction_id is a CharField with max_length=255. Let me search for any validation patterns or actual usage in the codebase.
Based on the search results, I can now generate a final response since I have enough context about how bank transaction IDs are handled in the codebase.
Bank transaction ID format is correctly implemented
The bank transaction ID is used specifically for grouping split credit card expenses when the configuration is set to 'CREDIT CARD CHARGE' and split expense grouping is 'MULTIPLE_LINE_ITEM'. The code handles both cases with and without bank transaction IDs appropriately. The format used in tests ('btxnqmATEEBkJ4') is valid as:
- The model defines it as CharField(max_length=255) with no format restrictions
- The codebase treats it as an optional identifier (null=True, blank=True)
- The ID is only used for grouping related expenses, not for external integrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for bank_transaction_id patterns in tests and production code
# to verify consistency with real-world format
echo "Searching for bank transaction ID patterns in tests..."
rg -g '!fixtures.py' 'bank_transaction_id.*btxn' tests/
echo "Searching for bank transaction ID validation patterns..."
ast-grep --pattern 'bank_transaction_id = models.$_'
Length of output: 354
Script:
#!/bin/bash
# Search for bank_transaction_id in models and validation code
echo "Searching for bank transaction ID field definitions..."
rg -A 3 'bank_transaction_id' --type python
echo "Searching for bank transaction ID patterns in test assertions..."
rg 'assert.*bank_transaction_id' tests/
echo "Looking for bank transaction ID format validation..."
ast-grep --pattern 'bank_transaction_id = $_'
Length of output: 28375
Script:
#!/bin/bash
# Search for bank transaction ID patterns in production code
echo "Searching for bank transaction ID patterns in production code..."
rg -g '!tests/*' 'bank_transaction_id' -A 3
echo "Searching for bank transaction ID validation or format checks..."
rg -g '!tests/*' 'btxn' -A 3
Length of output: 4019
apps/fyle/migrations/0035_support_split_expense_grouping.py (3)
1-11: LGTM! Migration metadata is well-structured.
The migration file follows Django conventions with proper dependencies and imports.
14-18: LGTM! Bank transaction ID field is properly defined.
The field definition allows for backward compatibility by being optional and has an appropriate length for storing transaction IDs.
19-23: Verify the default function implementation and document field constraints.
The split_expense_grouping field is non-nullable and relies on a default function. Please ensure:
- The
get_default_split_expense_groupingfunction is properly implemented - The migration script mentioned in PR objectives for existing organizations is executed before this migration
✅ Verification successful
Based on the shell script results, I can provide the final response as the verification is complete:
The default function implementation and field constraints are properly set up
- The
get_default_split_expense_grouping()function is correctly implemented inapps/fyle/models.pyand returns 'MULTIPLE_LINE_ITEM' as the default value. - The migration script for existing organizations exists at
scripts/sql/scripts/030-default-split-expense-grouping-for-old-orgs.sqlwhich sets the default value to 'SINGLE_LINE_ITEM'. - The field constraints are properly defined:
- Non-nullable CharField with max_length=100
- Valid choices: SINGLE_LINE_ITEM, MULTIPLE_LINE_ITEM
- Default value function is properly referenced
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the default function implementation and its usage
# Check the implementation of get_default_split_expense_grouping
ast-grep --pattern 'def get_default_split_expense_grouping'
# Check for any existing usage of split_expense_grouping
rg -A 5 'split_expense_grouping'
# Look for the migration script for existing organizations
fd -e py -e sql -e sh . | rg -l 'split_expense.*migration|disable.*split_expense'
Length of output: 12720
tests/test_fyle/conftest.py (1)
31-33: LGTM! Well-structured fixture declaration.
The fixture follows pytest best practices with clear naming and proper database dependency injection.
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (3)
78-79: LGTM: Response correctly mirrors the configuration
The response structure properly includes the new split_expense_grouping field with matching value.
29-30: Ensure comprehensive test coverage for split_expense_grouping configurations
While testing the new 'MULTIPLE_LINE_ITEM' configuration is good, we should also add test cases for:
- The default 'SINGLE_LINE_ITEM' configuration for legacy organizations
- The toggle functionality between different configurations
Let's verify if these test cases exist elsewhere:
Line range hint 1-1: Add test coverage for missing/invalid split_expense_grouping values
The new split_expense_grouping field is not included in the "missing values" test scenarios. Consider:
- Adding test cases for missing
split_expense_groupingvalue - Adding validation tests for invalid values
- Documenting whether this field is required or optional
This will ensure robust error handling and validation.
Let's check the serializer implementation:
apps/workspaces/apis/export_settings/serializers.py (1)
98-99: Verify update behavior for split_expense_grouping.
The field is correctly added to Meta.fields, but we should verify:
- How existing records are handled during updates
- If ExpenseGroupSettings.update_expense_group_settings method properly handles this field
✅ Verification successful
Field is properly handled in the update flow
The verification shows that:
- The field is properly defined in the model with a default value and choices (
SINGLE_LINE_ITEM,MULTIPLE_LINE_ITEM) - The update method in
ExpenseGroupSettings.update_expense_group_settingscorrectly handles this field in the update dictionary - The field is non-nullable and required in the serializer, ensuring data integrity
- Existing test coverage demonstrates proper handling of both values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if update_expense_group_settings method handles split_expense_grouping
ast-grep --pattern 'def update_expense_group_settings($params, $$$) {
$$$
}'
Length of output: 80
Script:
#!/bin/bash
# Let's try a different approach to find the update method and its handling
# First, let's locate the relevant files
fd "settings" --type f --extension py | grep -i "export\|expense"
# Then check for update methods in these files
rg "def update.*settings" --type py -A 10
Length of output: 1109
Script:
#!/bin/bash
# Let's check the ExpenseGroupSettings model to understand the field definition
rg "class ExpenseGroupSettings" --type py -B 2 -A 20
# Also check for any references to split_expense_grouping in the codebase
rg "split_expense_grouping" --type py -A 3 -B 3
Length of output: 18750
tests/test_netsuite/conftest.py (1)
272-274: LGTM! Verify test coverage for split expense grouping.
The change from singular to plural method name aligns with the PR objective of supporting split expense grouping. However, let's ensure comprehensive test coverage exists for this new functionality.
Let's verify the test coverage:
✅ Verification successful
Test coverage for split expense grouping is comprehensive
The codebase shows thorough test coverage for split expense grouping functionality:
- Multiple test cases in
tests/test_fyle/test_models.pyspecifically testingMULTIPLE_LINE_ITEMscenarios - Test fixtures in
tests/test_fyle/fixtures.pycovering multiple line item cases - Integration test data in SQL fixtures supporting multiple line item configurations
- Additional test coverage in
tests/test_netsuite/test_connector.pyfor multiple line items handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for split expense grouping functionality
# Look for test cases covering split expense scenarios
rg -l "split_expense" tests/
rg -l "create_credit_card_charge_lineitems.*multiple|many|split" tests/
# Check for any TODO comments related to testing
rg -i "todo.*test.*split|multiple" tests/
Length of output: 3057
tests/test_netsuite/test_connector.py (2)
292-293: LGTM! Variable naming aligns with multiple line items support.
The variable name change from singular to plural form correctly reflects the capability to handle multiple line items.
324-324: LGTM! Method call is correctly structured.
The test correctly passes the updated credit card charge and line items to the constructor.
apps/netsuite/tasks.py (1)
Line range hint 554-570: Consider handling attachments for all split expenses.
The code only loads attachments for the first expense in the group. For split expenses, this could result in missing attachments from other expenses in the group.
Consider modifying the attachment handling to process all expenses:
- expense = expense_group.expenses.first()
- refund = False
- if expense.amount < 0:
- refund = True
-
- attachment_link = load_attachments(netsuite_connection, expense, expense_group, credit_card_charge_object)
-
- if attachment_link:
- attachment_links[expense.expense_id] = attachment_link
+ refund = any(expense.amount < 0 for expense in expense_group.expenses.all())
+
+ for expense in expense_group.expenses.all():
+ attachment_link = load_attachments(netsuite_connection, expense, expense_group, credit_card_charge_object)
+ if attachment_link:
+ attachment_links[expense.expense_id] = attachment_linkAdditionally, consider adding validation for the line items before posting to ensure data integrity.
apps/netsuite/models.py (4)
687-698: LGTM! Method signature and setup are well-structured.
The method name change from singular to plural form accurately reflects its enhanced functionality to handle multiple line items.
708-718: LGTM! Employee mapping and class handling logic is well-structured.
The code follows a clear fallback pattern for determining the class_id, first checking employee mapping and then falling back to the class_id from expense group.
762-779: LGTM! Object creation logic is comprehensive.
The update_or_create operation properly handles all necessary fields and follows Django best practices.
781-783: LGTM! Return statement aligns with multiple items handling.
The method correctly returns a list of created line items, matching its purpose of handling multiple items.
apps/fyle/models.py (6)
76-76: Definition of SPLIT_EXPENSE_GROUPING constant is correct
The constant is properly defined and aligns with Django's choices field requirements.
109-109: Addition of bank_transaction_id field to Expense model is appropriate
The new field bank_transaction_id is correctly defined with appropriate attributes.
215-217: Default value function get_default_split_expense_grouping is properly implemented
The function correctly returns the default value for split_expense_grouping.
243-247: Addition of split_expense_grouping field to ExpenseGroupSettings model is appropriate
The new field is correctly defined with a default value and choices, ensuring proper configuration of expense grouping settings.
327-328: Ensuring split_expense_grouping is updated correctly in settings
Including split_expense_grouping in the update_or_create method ensures that the setting is saved and updated properly in ExpenseGroupSettings.
421-458: Grouping logic for corporate credit card expenses is correctly implemented
The code effectively handles grouping of corporate credit card expenses based on bank_transaction_id when split_expense_grouping is set to MULTIPLE_LINE_ITEM, ensuring expenses are appropriately grouped.
| COPY public.expenses (id, employee_email, category, sub_category, project, expense_id, expense_number, claim_number, amount, currency, foreign_amount, foreign_currency, settlement_id, reimbursable, state, vendor, cost_center, purpose, report_id, spent_at, approved_at, expense_created_at, expense_updated_at, created_at, updated_at, fund_source, custom_properties, verified_at, paid_on_netsuite, billable, org_id, tax_amount, tax_group_id, project_id, file_ids, corporate_card_id, is_skipped, report_title, employee_name, posted_at, accounting_export_summary, previous_export_state, workspace_id, paid_on_fyle, is_posted_at_null, bank_transaction_id) FROM stdin; | ||
| 1 [email protected] Accounts Payable Accounts Payable \N txjvDntD9ZXR E/2021/11/T/11 C/2021/11/R/5 50 USD \N \N set6GUp6tcEEp t PAYMENT_PROCESSING \N Treasury \N rpuN3bgphxbK 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 10:27:53.649+00 2021-11-15 10:28:46.775+00 2021-11-15 10:29:07.597095+00 2021-11-15 10:29:07.597111+00 PERSONAL {"Team": "", "Class": "", "Klass": "", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 2 [email protected] Accounts Payable Accounts Payable \N txy6folbrG2j E/2021/11/T/12 C/2021/11/R/6 100 USD \N \N setNVTcPkZ6on f PAYMENT_PROCESSING Ashwin Vendor \N \N rpHLA9Dfp9hN 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 13:11:22.304+00 2021-11-15 13:11:58.032+00 2021-11-15 13:12:12.250613+00 2021-11-15 13:12:12.250638+00 CCC {"Team": "", "Class": "", "Klass": "Klass", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 3 [email protected] Accounts Payable Accounts Payable \N txeLau9Rdu4X E/2021/11/T/1 C/2021/11/R/2 80 USD \N \N setqgvGQnsAya t PAYMENT_PROCESSING \N \N \N rpu5W0LYrk6e 2021-11-16 00:00:00+00 2021-11-16 00:00:00+00 2021-11-16 04:24:18.688+00 2021-11-16 04:25:21.996+00 2021-11-16 04:25:49.174565+00 2021-11-16 04:25:49.174584+00 PERSONAL {"Klass": "Klass", "Device Type": "", "Fyle Category": ""} \N f \N oraWFQlEpjbb 4.53 tg31j9m4PoEO \N \N \N f \N \N \N {} \N \N f f \N | ||
| 4 [email protected] Accounts Payable Accounts Payable \N txMLGb6Xy8m8 E/2021/11/T/2 C/2021/11/R/1 100 USD \N \N setqgvGQnsAya f PAYMENT_PROCESSING \N \N \N rprqDvARHUnv 2021-11-16 00:00:00+00 2021-11-16 00:00:00+00 2021-11-16 04:24:38.141+00 2021-11-16 04:25:21.996+00 2021-11-16 04:25:49.192351+00 2021-11-16 04:25:49.192367+00 CCC {"Device Type": "", "Fyle Category": ""} \N f \N oraWFQlEpjbb 16.67 tgSYjXsBCviv \N \N \N f \N \N \N {} \N \N f f \N | ||
| 173 [email protected] Food Food Project 2 tx7A5QpesrV5 E/2021/12/T/1 C/2021/12/R/1 120 USD \N \N set15sMvtRIiS t PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85N 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:30.076+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.685597+00 2021-12-03 11:26:58.685616+00 PERSONAL {} \N f \N orHe8CpW2hyN \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 174 [email protected] Food Food Project 2 txcKVVELn1Vl E/2021/12/T/2 C/2021/12/R/1 130 USD \N \N set15sMvtRIiS f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85N 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N orHe8CpW2hyN \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 600 [email protected] Food Food Project 2 txcKVVELn1Vlkill E/2021/12/T/298 C/2021/12/R/198 130 USD \N \N set15sMvtRIiSkill f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Nkill 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N | ||
| 601 [email protected] Food Food Project 2 txcKVVELn1Vlgon E/2021/12/T/299 C/2021/12/R/199 130 USD \N \N set15sMvtRIiSgon f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Ngon 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test data for bank_transaction_id
The test data doesn't include any values for bank_transaction_id. Consider adding test cases with bank transaction IDs to validate the split expense grouping functionality.
Add test data with bank transaction IDs:
1 [email protected] Accounts Payable Accounts Payable \N txjvDntD9ZXR E/2021/11/T/11 C/2021/11/R/5 50 USD \N \N set6GUp6tcEEp t PAYMENT_PROCESSING \N Treasury \N rpuN3bgphxbK 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 10:27:53.649+00 2021-11-15 10:28:46.775+00 2021-11-15 10:29:07.597095+00 2021-11-15 10:29:07.597111+00 PERSONAL {"Team": "", "Class": "", "Klass": "", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f BTX123
+2 [email protected] Accounts Payable Accounts Payable \N txy6folbrG2j E/2021/11/T/12 C/2021/11/R/6 100 USD \N \N setNVTcPkZ6on f PAYMENT_PROCESSING Ashwin Vendor \N \N rpHLA9Dfp9hN 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 13:11:22.304+00 2021-11-15 13:11:58.032+00 2021-11-15 13:12:12.250613+00 2021-11-15 13:12:12.250638+00 CCC {"Team": "", "Class": "", "Klass": "Klass", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f BTX123Committable suggestion skipped: line range outside the PR's diff.
| COPY public.expense_group_settings (id, reimbursable_expense_group_fields, corporate_credit_card_expense_group_fields, expense_state, reimbursable_export_date_type, created_at, updated_at, workspace_id, import_card_credits, ccc_export_date_type, ccc_expense_state, split_expense_grouping) FROM stdin; | ||
| 1 {employee_email,report_id,claim_number,fund_source} {employee_email,report_id,claim_number,fund_source} PAYMENT_PROCESSING current_date 2021-11-15 08:46:16.069944+00 2021-11-15 08:46:16.069986+00 1 f current_date PAID MULTIPLE_LINE_ITEM | ||
| 2 {fund_source,employee_email,settlement_id,spent_at} {expense_id,fund_source,employee_email,settlement_id,spent_at} PAID spent_at 2021-11-16 04:16:57.847694+00 2021-11-16 07:34:26.302812+00 2 f spent_at PAID MULTIPLE_LINE_ITEM | ||
| 74 {employee_email,report_id,claim_number,fund_source} {claim_number,employee_email,expense_id,report_id,fund_source} PAYMENT_PROCESSING last_spent_at 2021-12-03 11:00:33.637654+00 2021-12-03 11:04:00.206339+00 49 f last_spent_at PAID MULTIPLE_LINE_ITEM |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test data coverage for split_expense_grouping
The test data only includes 'MULTIPLE_LINE_ITEM' for split_expense_grouping. Consider adding test cases with 'SINGLE_LINE_ITEM' to ensure both scenarios are covered.
Add varied test data:
1 {employee_email,report_id,claim_number,fund_source} {employee_email,report_id,claim_number,fund_source} PAYMENT_PROCESSING current_date 2021-11-15 08:46:16.069944+00 2021-11-15 08:46:16.069986+00 1 f current_date PAID MULTIPLE_LINE_ITEM
+2 {employee_email,report_id,claim_number,fund_source} {employee_email,report_id,claim_number,fund_source} PAYMENT_PROCESSING current_date 2021-11-15 08:46:16.069944+00 2021-11-15 08:46:16.069986+00 1 f current_date PAID SINGLE_LINE_ITEMCommittable suggestion skipped: line range outside the PR's diff.
| 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 (lines 42 and 44), which could lead to data redundancy or processing issues.
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'
+ 'spent_at'
]📝 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
Add validation and documentation for split_expense_grouping field.
The new required field needs additional constraints:
- Define choices/validation for allowed values
- Add docstring explaining valid values and their implications
- Consider making it optional with a default value for backward compatibility
- split_expense_grouping = serializers.CharField(allow_null=False, allow_blank=False, required=True)
+ SPLIT_EXPENSE_GROUPING_CHOICES = [
+ ('SINGLE_LINE_ITEM', 'Single Line Item'),
+ ('MULTIPLE_LINE_ITEMS', 'Multiple Line Items'),
+ ]
+
+ split_expense_grouping = serializers.ChoiceField(
+ choices=SPLIT_EXPENSE_GROUPING_CHOICES,
+ required=True,
+ help_text='Controls how split expenses are grouped in exports: '
+ 'SINGLE_LINE_ITEM for legacy behavior, '
+ 'MULTIPLE_LINE_ITEMS for grouping split expenses together'
+ )📝 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.
| split_expense_grouping = serializers.CharField(allow_null=False, allow_blank=False, required=True) | |
| SPLIT_EXPENSE_GROUPING_CHOICES = [ | |
| ('SINGLE_LINE_ITEM', 'Single Line Item'), | |
| ('MULTIPLE_LINE_ITEMS', 'Multiple Line Items'), | |
| ] | |
| split_expense_grouping = serializers.ChoiceField( | |
| choices=SPLIT_EXPENSE_GROUPING_CHOICES, | |
| required=True, | |
| help_text='Controls how split expenses are grouped in exports: ' | |
| 'SINGLE_LINE_ITEM for legacy behavior, ' | |
| 'MULTIPLE_LINE_ITEMS for grouping split expenses together' | |
| ) |
| def test_split_expense_grouping_pairs_of_same_ids(db, update_config_for_split_expense_grouping): | ||
| ''' | ||
| Test for grouping of 2 pairs of expenses with the same bank transaction ids | ||
| ''' | ||
| 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'][:4] | ||
| expenses[0]['bank_transaction_id'] = 'sample_1' | ||
| expenses[1]['bank_transaction_id'] = 'sample_1' | ||
| expenses[2]['bank_transaction_id'] = 'sample_2' | ||
| expenses[3]['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) == 4, f'Expected 4 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) == 4, f'Expected 4 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
Reduce code duplication and enhance assertions
- The test setup code is duplicated across all three test functions
- Like the previous test, this one also lacks verification of group contents
Consider extracting common setup code into a fixture:
@pytest.fixture
def setup_split_expense_grouping(db, update_config_for_split_expense_grouping):
def _setup(workspace_id, expenses, bank_transaction_ids):
# 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 expenses
for expense, bank_id in zip(expenses, bank_transaction_ids):
expense['bank_transaction_id'] = bank_id
Expense.create_expense_objects(expenses, workspace_id=workspace_id)
expense_objects = Expense.objects.filter(
expense_id__in=[expense['id'] for expense in expenses]
)
return configuration, expense_group_settings, expense_objects
return _setupThen simplify the test:
-def test_split_expense_grouping_pairs_of_same_ids(db, update_config_for_split_expense_grouping):
+def test_split_expense_grouping_pairs_of_same_ids(setup_split_expense_grouping):
'''
Test for grouping of 2 pairs of expenses with the same bank transaction ids
'''
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'][:4]
- expenses[0]['bank_transaction_id'] = 'sample_1'
- expenses[1]['bank_transaction_id'] = 'sample_1'
- expenses[2]['bank_transaction_id'] = 'sample_2'
- expenses[3]['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])
+ bank_ids = ['sample_1', 'sample_1', 'sample_2', 'sample_2']
+ configuration, expense_group_settings, expense_objects = setup_split_expense_grouping(
+ workspace_id, expenses, bank_ids
+ )
assert len(expense_objects) == 4, 'Should create exactly 4 expense objects'
# Test grouping logic and add assertions for group contents...Add assertions to verify group contents:
# Verify group contents in MULTIPLE_LINE_ITEM mode
new_groups = groups[old_count:]
assert len(new_groups) == 2, 'Should create exactly 2 groups'
for group in new_groups:
expenses = group.expenses.all()
assert len(expenses) == 2, 'Each group should contain exactly 2 expenses'
bank_ids = {expense.bank_transaction_id for expense in expenses}
assert len(bank_ids) == 1, 'All expenses in a group should have the same bank_transaction_id'| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format( | ||
| lineitem.category, lineitem.sub_category) | ||
|
|
||
| if expense_group.fund_source == 'CCC' and general_mappings.use_employee_department and \ | ||
| general_mappings.department_level in ('ALL', 'TRANSACTION_LINE'): | ||
| employee_mapping = EmployeeMapping.objects.filter( | ||
| source_employee__value=expense_group.description.get('employee_email'), | ||
| account = CategoryMapping.objects.filter( | ||
| source_category__value=category, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix None comparison in category handling.
Use Python's identity operator is when comparing with None for better readability and performance.
-category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format(
+category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) else '{0} / {1}'.format(📝 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.
| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format( | |
| lineitem.category, lineitem.sub_category) | |
| if expense_group.fund_source == 'CCC' and general_mappings.use_employee_department and \ | |
| general_mappings.department_level in ('ALL', 'TRANSACTION_LINE'): | |
| employee_mapping = EmployeeMapping.objects.filter( | |
| source_employee__value=expense_group.description.get('employee_email'), | |
| account = CategoryMapping.objects.filter( | |
| source_category__value=category, | |
| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) else '{0} / {1}'.format( | |
| lineitem.category, lineitem.sub_category) | |
| account = CategoryMapping.objects.filter( | |
| source_category__value=category, |
🧰 Tools
🪛 Ruff (0.7.0)
700-700: Comparison to None should be cond is None
Replace with cond is None
(E711)
| billable = lineitem.billable | ||
| if customer_id: | ||
| if not billable: | ||
| billable = False | ||
| else: |
There was a problem hiding this comment.
💡 Codebase verification
Based on the context gathered, I can now provide the final response:
The billable flag handling is inconsistent across different models
The code shows three different implementations for handling the billable flag when there's no customer_id:
- For BillLineitem: Sets
billable = None - For CreditCardChargeLineItem: Sets
billable = False - For ExpenseReportLineItem: Sets
billable = None
This inconsistency in billable flag handling across different transaction types needs to be addressed to maintain consistent business logic.
apps/netsuite/models.py: Standardize the billable flag behavior whencustomer_idis None across all three models (BillLineitem, CreditCardChargeLineItem, and ExpenseReportLineItem)
🔗 Analysis chain
Verify the change in billable expense logic.
The code now sets billable = False when there's no customer_id, which might affect existing expense processing. Please confirm if this change in business logic is intentional and won't impact existing integrations.
Let me gather more context about the billable flag usage.
Let me check the complete context of this billable flag handling in the models.py file.
Let me try a different approach to get the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of billable flag
rg -l 'billable.*=.*None' --type py
Length of output: 96
Script:
#!/bin/bash
# Check the actual usage of billable flag in both files
rg 'billable.*=.*None|billable.*=.*False' -B 2 -A 2 --type py
Length of output: 3826
Script:
#!/bin/bash
# Get more context around the billable flag handling in models.py
rg -B 10 -A 10 'if customer_id.*billable' apps/netsuite/models.py
Length of output: 67
Script:
#!/bin/bash
# Get the complete context around the billable flag handling
rg -B 15 -A 15 'billable = lineitem\.billable' apps/netsuite/models.py
Length of output: 4633
| @@ -173,6 +177,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id, skip_update: bool | |||
| 'purpose': expense['purpose'], | |||
| 'report_id': expense['report_id'], | |||
| 'corporate_card_id': expense['corporate_card_id'], | |||
| 'bank_transaction_id': expense['bank_transaction_id'], | |||
There was a problem hiding this comment.
Handle potential KeyError when accessing expense['bank_transaction_id']
Accessing expense['bank_transaction_id'] without checking may raise a KeyError if 'bank_transaction_id' is missing in the expense dictionary. Consider using expense.get('bank_transaction_id') to prevent possible exceptions.
Apply this diff to fix the issue:
-'bank_transaction_id': expense['bank_transaction_id'],
+'bank_transaction_id': expense.get('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.
| 'bank_transaction_id': expense['bank_transaction_id'], | |
| 'bank_transaction_id': expense.get('bank_transaction_id'), |
| for credit_card_charge_lineitem in credit_card_charge_lineitems: | ||
| credit_card_charge_lineitem.amount = abs(credit_card_charge_lineitem.amount) | ||
| credit_card_charge_lineitem.save() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Modifying and Saving Line Items Within Refund Logic
Modifying and saving credit_card_charge_lineitem.amount directly within the refund logic may lead to unintended side effects by altering the database state. It's better to adjust the amount in the payload without persisting changes to the database.
Apply this diff:
-for credit_card_charge_lineitem in credit_card_charge_lineitems:
- credit_card_charge_lineitem.amount = abs(credit_card_charge_lineitem.amount)
- credit_card_charge_lineitem.save()
+for line_item in credit_card_charge_lineitems:
+ line_item.amount = abs(line_item.amount)
+ # Avoid saving changes to the database to prevent side effectsAlternatively, create a copy of the line items list and modify the copy to ensure the original data remains unchanged.
📝 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.
| for credit_card_charge_lineitem in credit_card_charge_lineitems: | |
| credit_card_charge_lineitem.amount = abs(credit_card_charge_lineitem.amount) | |
| credit_card_charge_lineitem.save() | |
| for line_item in credit_card_charge_lineitems: | |
| line_item.amount = abs(line_item.amount) | |
| # Avoid saving changes to the database to prevent side effects |
| }, | ||
| } | ||
| for line in credit_card_charge_lineitems: | ||
| expense = Expense.objects.get(pk=line.expense_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Error Handling When Retrieving Expense Object
The code retrieves the Expense object without handling the possibility that it may not exist, which could raise a DoesNotExist exception and cause a crash. Consider adding error handling to manage this scenario gracefully.
Apply this diff to handle the exception:
+from django.core.exceptions import ObjectDoesNotExist
...
- expense = Expense.objects.get(pk=line.expense_id)
+ try:
+ expense = Expense.objects.get(pk=line.expense_id)
+ except Expense.DoesNotExist:
+ # Handle the missing expense, e.g., log an error or skip this line item
+ continueCommittable suggestion skipped: line range outside the PR's diff.
* 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)
Description
split_expense_groupingoptionClickup
https://app.clickup.com/t/86cx0vh5j
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 (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_groupingin export settings (feat: support GET and PUT forsplit_expense_groupingin export settings #672)feat: support GET and PUT for
split_expense_groupingin export settingsfeat: implement split expense grouping functionality (feat: implement split expense grouping functionality #675)
feat: implement split expense grouping functionality
feat: add script for split expense grouping (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 (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
forloopSummary by CodeRabbit
Release Notes
New Features
bank_transaction_idfor tracking bank transactions in expenses.split_expense_groupingto enhance expense categorization options.Bug Fixes
Tests
These enhancements improve expense management and tracking, providing users with greater flexibility and accuracy in their financial records.