test: write unit tests and add fixtures for split expense grouping #670
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new pytest fixture for configuring split expense grouping in the test suite. It updates expense-related settings, including attributes for corporate credit card expenses and expense group fields. Additionally, new test functions are added to enhance coverage for expense grouping based on bank transaction IDs, validating various scenarios. Changes also include the addition of a new key in the test data dictionary to represent detailed expense entries. Furthermore, multiple modifications are made across models, serializers, and connectors to support the handling of multiple credit card charge line items. 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 13 minutes and 26 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 (
|
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
tests/test_fyle/conftest.py (1)
31-47: Enhance code quality with type hints and documentationConsider adding type hints and a docstring to improve code maintainability and clarity.
Apply this diff to add type hints and documentation:
@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(db) -> callable: + """Fixture to update configuration for split expense grouping tests. + + Args: + db: pytest fixture for database access + + Returns: + callable: A function that updates general and expense group settings + """ + def _update_config_for_split_expense_grouping( + general_settings: 'GeneralSettings', + expense_group_settings: 'ExpenseGroupSettings' + ) -> None:tests/test_fyle/test_models.py (4)
134-168: LGTM! Consider enhancing test coverage with additional assertions.The test structure and implementation look good. Consider these enhancements:
- Verify the contents of expense groups, not just their count
- Add edge cases (e.g., empty expenses list, invalid expense data)
Add assertions to verify expense group contents:
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)}' +# Verify each group contains exactly one expense +for group in groups: + assert group.expenses.count() == 1, f'Expected 1 expense in group, got {group.expenses.count()}'
170-207: LGTM! Consider verifying expense grouping relationships.The test structure is good, but could be enhanced by verifying:
- Which expenses are grouped together in MULTIPLE_LINE_ITEM mode
- The ordering of expenses within groups
Add assertions to verify grouping relationships:
groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}' +# Verify grouping relationships +for group in groups.order_by('-created_at')[:2]: # Get the newly created groups + expenses_in_group = group.expenses.all() + if expenses_in_group.first().bank_transaction_id == 'sample_1': + assert expenses_in_group.count() == 2, 'Expected 2 expenses with sample_1' + else: + assert expenses_in_group.count() == 1, 'Expected 1 expense with sample_2'
209-245: Fix spacing in assertion message and enhance group content verification.The test looks good but has a minor spacing issue and could benefit from additional verifications.
Fix the spacing in the assertion message and add group content verification:
- assert len(groups) - old_count== 2, f'Expected 2 groups, got {len(groups) - old_count}' + assert len(groups) - old_count == 2, f'Expected 2 groups, got {len(groups) - old_count}' + + # Verify group contents + new_groups = groups.order_by('-created_at')[:2] + for group in new_groups: + assert group.expenses.count() == 2, 'Each group should contain exactly 2 expenses' + expenses_in_group = group.expenses.all() + # All expenses in a group should have the same bank_transaction_id + assert len(set(e.bank_transaction_id for e in expenses_in_group)) == 1
134-245: Consider refactoring common setup code into helper methods.The test functions share similar setup patterns that could be extracted into helper methods to improve maintainability and reduce duplication.
Consider creating these helper methods:
def setup_test_configuration(workspace_id): """Setup and return test configuration and 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) return configuration, expense_group_settings def create_test_expenses(expenses, workspace_id): """Create and return test expense objects""" Expense.create_expense_objects(expenses, workspace_id=workspace_id) return Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) def verify_expense_groups(expenses, expected_count, message): """Verify expense group count and return groups""" groups = ExpenseGroup.objects.filter(expenses__expense_id__in=[expense['id'] for expense in expenses]) assert len(groups) == expected_count, f'Expected {expected_count} groups, got {len(groups)}' return groups
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/fixtures.py(2 hunks)tests/test_fyle/test_models.py(2 hunks)
🧰 Additional context used
🪛 Ruff
tests/test_fyle/fixtures.py
696-696: Dictionary key literal 'bank_transaction_id' repeated
(F601)
736-736: Dictionary key literal 'bank_transaction_id' repeated
(F601)
776-776: Dictionary key literal 'bank_transaction_id' repeated
(F601)
816-816: Dictionary key literal 'bank_transaction_id' repeated
(F601)
🔇 Additional comments (1)
tests/test_fyle/fixtures.py (1)
1-2: LGTM: Clean import addition
The datetime import is appropriately added at the top of the file and is used in the test fixtures.
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/test_fyle/fixtures.py (1)
657-814: Consider documenting the test data structure.The test data would benefit from a docstring explaining:
- The purpose of the "ccc_split_expenses" dataset
- The relationship between the expenses (same bank transaction ID)
- The significance of the sequential amounts
Add a docstring like this at the start of the data dictionary:
data = { + # Test fixtures for corporate credit card split expenses + # Contains sets of expenses that share the same bank_transaction_id + # to test expense grouping functionality "ccc_split_expenses": [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/test_fyle/fixtures.py(2 hunks)
🔇 Additional comments (2)
tests/test_fyle/fixtures.py (2)
1-2: LGTM: Clean import statement.
The datetime import is appropriately placed at the top of the file and is used consistently throughout the test fixtures.
657-814: Verify test data coverage for split expense scenarios.
The test data effectively covers a basic split expense scenario with 4 expenses sharing the same bank transaction ID. However, consider adding additional test cases for:
- Edge cases:
- Split expenses with different dates
- Split expenses with different currencies
- Split expenses with different states
- Error cases:
- Invalid/missing bank transaction IDs
- Negative amounts
- Zero amounts
Let's verify if these scenarios are covered elsewhere in the test suite:
Would you like me to help generate additional test fixtures for these scenarios?
✅ Verification successful
Let me check the content of these files to verify the test coverage for split expense scenarios.
Let me check for test cases with different currencies and states.
Let me check for test cases with negative or zero amounts.
Based on the analysis of the test files, I can now provide a final response:
Test coverage for split expense scenarios is already comprehensive
The codebase already includes test coverage for the suggested scenarios:
- Different states: Tests cover both APPROVED and PAYMENT_PROCESSING states
- Invalid/missing bank transaction IDs: Test case
test_split_expense_grouping_with_no_bank_transaction_idexists - Zero amounts: Multiple test fixtures with
amount: 0 - Negative amounts: Test fixture with
amount: -99 - Multiple bank transaction IDs: Tests with different bank transaction IDs ('sample_1', 'sample_2')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test cases related to split expenses
rg -l "bank_transaction_id" tests/
Length of output: 93
Script:
#!/bin/bash
# Search for test cases and fixtures related to split expenses with different scenarios
rg -B 5 -A 5 "bank_transaction_id" tests/test_fyle/test_models.py tests/test_fyle/fixtures.py
Length of output: 6744
Script:
#!/bin/bash
# Search for test cases with different currencies and states in split expenses
rg -B 2 -A 2 "'currency':|'state':|'spent_at':" tests/test_fyle/fixtures.py
Length of output: 30204
Script:
#!/bin/bash
# Search for test cases with negative or zero amounts
rg -B 2 -A 2 "'amount': -|'amount': 0" tests/test_fyle/fixtures.py tests/test_fyle/test_models.py
Length of output: 2050
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/test_fyle/conftest.py (1)
34-36: Consider adding type hints and docstringAdding type hints and documentation would improve the maintainability of this test fixture.
Apply this diff to add type hints and documentation:
- def _update_config_for_split_expense_grouping(general_settings, expense_group_settings): + def _update_config_for_split_expense_grouping( + general_settings: GeneralSettings, + expense_group_settings: ExpenseGroupSettings + ) -> None: + """ + Updates configuration for split expense grouping test scenarios. + + Args: + general_settings: General settings instance to update + expense_group_settings: Expense group settings instance to update + """ general_settings.corporate_credit_card_expenses_object = 'CREDIT CARD CHARGE'tests/test_fyle/test_models.py (4)
134-168: Consider adding test cases for empty string bank transaction IDsThe test effectively covers null bank transaction IDs, but consider adding test cases for empty string bank transaction IDs (
"") to ensure robust handling of all falsy values.# Additional test case suggestion expenses[0]['bank_transaction_id'] = '' expenses[1]['bank_transaction_id'] = ''
170-207: Add test cases for malformed bank transaction IDsWhile the test covers valid bank transaction IDs well, consider adding test cases for:
- Malformed/invalid bank transaction ID formats
- Special characters in bank transaction IDs
# Additional test cases suggestion expenses[0]['bank_transaction_id'] = 'sample@#$%' expenses[1]['bank_transaction_id'] = 'sample@#$%' expenses[2]['bank_transaction_id'] = 'sample_with_very_long_id_' * 10 # Test long IDs
209-245: Add test cases for larger expense groupsConsider adding test cases with a larger number of expenses (e.g., 10+ expenses) sharing the same bank transaction ID to verify performance and correctness with bigger datasets.
140-143: Refactor common configuration setup codeThere's duplicate configuration setup code across all three test functions. Consider moving this common setup to a helper function or extending the
update_config_for_split_expense_groupingfixture to handle this.def setup_split_expense_config(workspace_id): 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) return configuration, expense_group_settingsAlso applies to: 177-180, 215-218
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/test_models.py(2 hunks)
🔇 Additional comments (2)
tests/test_fyle/conftest.py (2)
31-47: Verify test configuration values
Please ensure that the following configuration values align with the actual test requirements:
- 'CREDIT CARD CHARGE' as the corporate_credit_card_expenses_object
- 'SINGLE_LINE_ITEM' as the split_expense_grouping value
- The list of required fields in corporate_credit_card_expense_group_fields
37-45:
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 unexpected behavior in the tests.
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'
]|
|
||
| @pytest.fixture | ||
| def update_config_for_split_expense_grouping(db): | ||
| def _update_config_for_split_expense_grouping(general_settings, expense_group_settings): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation to inner function
The function should validate its input parameters to fail fast and provide clear error messages.
Apply this diff at the start of the function:
def _update_config_for_split_expense_grouping(general_settings, expense_group_settings):
+ if not general_settings or not expense_group_settings:
+ raise ValueError("Both general_settings and expense_group_settings must be provided")
+
general_settings.corporate_credit_card_expenses_object = 'CREDIT CARD CHARGE'📝 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 _update_config_for_split_expense_grouping(general_settings, expense_group_settings): | |
| def _update_config_for_split_expense_grouping(general_settings, expense_group_settings): | |
| if not general_settings or not expense_group_settings: | |
| raise ValueError("Both general_settings and expense_group_settings must be provided") | |
| general_settings.corporate_credit_card_expenses_object = 'CREDIT CARD CHARGE' |
JustARatherRidiculouslyLongUsername
force-pushed
the
ut-split-expense-grouping
branch
from
2456e40 to
a54d312
Compare
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (26)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
1153-1154: Consider adding NOT NULL constraint to bank_transaction_idThe
bank_transaction_idcolumn is nullable, but since it's used for split expense grouping, consider if it should be NOT NULL to ensure data integrity.
11584-11587: Verify test coverage for split expense grouping valuesAll test records use 'MULTIPLE_LINE_ITEM' for split_expense_grouping. Consider adding test cases for other possible values to ensure comprehensive test coverage.
tests/test_fyle/fixtures.py (1)
667-822: Consider adding documentation for the test scenario.The new
ccc_split_expensestest data is well-structured with 4 expenses sharing the same bank transaction ID but different amounts (1,2,3,4). This provides good coverage for testing split expense grouping logic.Consider adding a comment above the test data to document:
- The purpose of this specific test scenario
- What it's testing (e.g., "Testing multiple line items split from same bank transaction")
- Expected grouping behavior
apps/fyle/migrations/0035_support_split_expense_grouping.py (1)
14-18: Consider adding an index and standardizing NULL handling for bank_transaction_idThe
bank_transaction_idfield might benefit from the following improvements:
- Add a database index if this field will be used for lookups
- Standardize NULL handling by choosing either
null=Trueorblank=True, but not both, to avoid storing empty values in two different waysConsider applying this modification:
migrations.AddField( model_name='expense', name='bank_transaction_id', - field=models.CharField(blank=True, help_text='Bank Transaction ID', max_length=255, null=True), + field=models.CharField(blank=True, help_text='Bank Transaction ID', max_length=255, null=True, db_index=True), ),apps/workspaces/apis/export_settings/serializers.py (2)
98-99: Maintain consistent field ordering and indentationConsider:
- Following a consistent ordering pattern (e.g., alphabetical or logical grouping)
- Maintaining consistent indentation with other fields
'corporate_credit_card_expense_group_fields', 'ccc_export_date_type', - 'ccc_expense_state', - 'split_expense_grouping' + 'ccc_expense_state', + 'split_expense_grouping'
Line range hint
134-186: Consider adding explicit validation for split_expense_groupingThe
updatemethod implicitly handles the new field through theExpenseGroupSettings.update_expense_group_settingscall, but consider:
- Adding explicit validation in the
validatemethod to ensure valid values- Adding default handling in the
updatemethod similar to other expense group settings fieldsif not expense_group_settings['ccc_export_date_type']: expense_group_settings['ccc_export_date_type'] = 'current_date' + if not expense_group_settings['split_expense_grouping']: + expense_group_settings['split_expense_grouping'] = 'default_value' # Replace with appropriate default + ExpenseGroupSettings.update_expense_group_settings(expense_group_settings, workspace_id=workspace_id)tests/test_fyle/test_models.py (2)
134-168: Add test case for empty string bank transaction IDsWhile the test covers null bank transaction IDs well, consider adding a test case for empty string bank transaction IDs (
"") to ensure complete coverage of edge cases.def test_split_expense_grouping_with_no_bank_transaction_id(db, update_config_for_split_expense_grouping): # ... existing test code ... + + # Test with empty string bank transaction IDs + for expense in expenses: + expense['bank_transaction_id'] = "" + + Expense.create_expense_objects(expenses, workspace_id=workspace_id) + expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) + + 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 for empty string bank_transaction_ids, got {len(groups)}'
209-245: Add stress test for large number of expensesConsider adding a stress test case with a large number of expenses sharing the same bank transaction ID to verify performance and stability.
+def test_split_expense_grouping_stress_test(db, update_config_for_split_expense_grouping): + ''' + Test grouping of a large number of expenses with the same 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) + + # Create 100 expenses with same bank_transaction_id + base_expense = data['ccc_split_expenses'][0] + expenses = [] + for i in range(100): + expense = base_expense.copy() + expense['id'] = f'tx_{i}' + expense['bank_transaction_id'] = 'stress_test_id' + expenses.append(expense) + + Expense.create_expense_objects(expenses, workspace_id=workspace_id) + expense_objects = Expense.objects.filter(expense_id__in=[expense['id'] for expense in expenses]) + + # Test grouping performance + 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) == 100, f'Expected 100 groups for SINGLE_LINE_ITEM, got {len(groups)}'tests/test_netsuite/conftest.py (2)
81-83: Consider refactoring workspace ID assignments for better maintainabilityThe workspace ID updates are duplicated across multiple fixtures with hardcoded values. This could make maintenance difficult if workspace IDs need to change.
Consider:
- Creating a helper function to handle workspace ID updates
- Using workspace ID constants or fixture parameters instead of hardcoded values
+ def _update_expense_workspace_ids(expense_group, workspace_id): + for expense in expense_group.expenses.all(): + expense.workspace_id = workspace_id + expense.save() def create_expense_report(db, add_netsuite_credentials, add_fyle_credentials): expense_group = ExpenseGroup.objects.filter(workspace_id=1).first() - for expense in expense_group.expenses.all(): - expense.workspace_id = 1 - expense.save() + _update_expense_workspace_ids(expense_group, workspace_id=1)Also applies to: 95-97, 252-254, 266-268
308-324: Enhance tax destination attributes test dataA few suggestions to improve the test data setup:
- The ID generation (
98765+i) could potentially collide with other test data. Consider using a more robust ID generation strategy:- id = 98765+i, + id = f"test_tax_item_{i}", # or use a proper test ID generation utility
- Consider adding more diverse test data to cover different scenarios:
detail = { - 'tax_rate': 5.0 + 'tax_rate': 5.0 + i # Different rates per workspace },
- Consider parameterizing the fixture to make it more flexible for different test scenarios:
@pytest.fixture def add_tax_destination_attributes(db, workspace_ids=None, tax_rates=None): workspace_ids = workspace_ids or [1, 2, 49] tax_rates = tax_rates or [5.0, 7.5, 10.0] for workspace_id, tax_rate in zip(workspace_ids, tax_rates): DestinationAttribute.objects.create( attribute_type='TAX_ITEM', value=f'Tax_{tax_rate}', destination_id='103578', active=True, detail={'tax_rate': tax_rate}, workspace_id=workspace_id )apps/netsuite/models.py (1)
118-129: LGTM! Consider adding type hints for better maintainability.The changes improve the logic flow by first checking for tax settings before attempting to get tax mappings. However, there are some opportunities for improvement:
Consider applying these enhancements:
-def get_tax_item_id_or_none(expense_group: ExpenseGroup, general_mapping: GeneralMapping, lineitem: Expense = None): +def get_tax_item_id_or_none( + expense_group: ExpenseGroup, + general_mapping: GeneralMapping, + lineitem: Expense = None +) -> Optional[str]: tax_code = None 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 return tax_codeThe changes:
- Add return type hint
- Split long function signature for better readability
- Remove extra blank line at line 122
- Add
Optionaltype hint to indicate that the function may returnNonetests/test_netsuite/test_tasks.py (3)
Line range hint
1-1000: Improve test data handling and isolationThe test suite could benefit from several improvements to make it more maintainable and reliable:
- Use factory patterns for test data generation instead of hardcoded IDs
- Implement proper cleanup after test data modifications
- Isolate tests to prevent shared state issues
Consider implementing a test data factory:
@pytest.fixture def expense_factory(): def _create_expense(**kwargs): defaults = { 'workspace_id': 1, 'fund_source': 'PERSONAL', 'settlement_id': 'test_settlement', # Add other default values } defaults.update(kwargs) return Expense.objects.create(**defaults) return _create_expense
Line range hint
1-1000: Add test coverage for additional error scenariosThe test suite would benefit from additional test cases covering:
- Network timeouts
- Rate limiting scenarios
- Partial failure scenarios
- Concurrent execution issues
Example test case for timeout handling:
def test_post_bill_timeout(mocker, db): mocker.patch( 'netsuitesdk.api.vendor_bills.VendorBills.post', side_effect=requests.exceptions.Timeout ) workspace_id = 1 task_log = TaskLog.objects.filter(workspace_id=workspace_id).first() task_log.status = 'READY' task_log.save() expense_group = ExpenseGroup.objects.filter(workspace_id=workspace_id).first() create_bill(expense_group, task_log.id, True) task_log.refresh_from_db() assert task_log.status == 'FAILED' assert 'Request timed out' in task_log.detail['message']
Line range hint
1-1000: Improve test organization and maintainabilityThe test file has grown quite large and could benefit from better organization:
- Split into smaller, focused test files (e.g.,
test_bill_creation.py,test_vendor_payments.py)- Use test classes to group related tests
- Make test names more descriptive of the scenarios they test
Example reorganization:
# test_bill_creation.py class TestBillCreation: def test_successful_bill_creation_with_valid_data(self): pass def test_bill_creation_fails_with_invalid_vendor(self): pass # test_vendor_payments.py class TestVendorPayments: def test_successful_vendor_payment_creation(self): pass def test_vendor_payment_fails_with_invalid_bill(self): passapps/fyle/models.py (1)
421-453: Refactor CCC expense grouping logic for clarity and maintainabilityThe logic for grouping corporate credit card (CCC) expenses based on
bank_transaction_idwithin the condition wheresplit_expense_groupingis'MULTIPLE_LINE_ITEM'adds complexity to thecreate_expense_groups_by_report_id_fund_sourcemethod. Consider refactoring this logic into a separate method or simplifying the nested conditions to improve readability and maintainability.tests/test_netsuite/test_connector.py (4)
32-75: Refactor to reduce code duplication intest_construct_expense_report_with_tax_balancingThe function contains repeated code blocks for different tax balancing scenarios. Consider using
pytest.mark.parametrizeor helper functions to reduce duplication and improve maintainability.
115-156: Refactortest_construct_bill_item_for_tax_balancingto minimize duplicationThe test repeats similar code blocks for different scenarios. Refactoring using parameterization or helper functions can enhance readability and maintainability.
234-284: Refactortest_construct_journal_entry_with_tax_balancingto reduce redundancyThis test function has repetitive code for various tax balancing scenarios. Consider refactoring to avoid duplication.
301-341: Refactortest_contruct_credit_card_charge_with_tax_balancingto streamline testsThe function contains repeated code blocks. Using parameterized tests can make the code cleaner and more maintainable.
apps/netsuite/tasks.py (4)
329-331: Ensure consistent use of keyword arguments in function callsIn lines 329-331, the calls to
construct_linesuse keyword arguments inconsistently. For better readability and maintenance, consider standardizing the use of keyword arguments and their order in these function calls.
344-344: Verify parameter order consistency in function callAt line 344, the
construct_linesfunction is called with parameters in a different order compared to previous calls. For consistency and to prevent potential errors, ensure that the parameter order matches the function definition and is consistent across all calls.
615-615: Maintain consistent variable naming withgeneral_mappingsAt line 615, the variable
general_mappingis assigned. To keep naming consistent throughout the codebase, consider renaming it togeneral_mappings.
680-680: Consistent variable naming forgeneral_mappingsAt line 680,
general_mappingis used again. To maintain consistency, consider changing it togeneral_mappingsas used elsewhere.apps/netsuite/connector.py (3)
Line range hint
1236-1283: Refactor hardcoded script IDs into constantsThe script IDs like
'custcolfyle_receipt_link','custcolfyle_receipt_link_2','custcolfyle_expense_url', and'custcolfyle_expense_url_2'are hardcoded within theprepare_custom_segmentsmethod. Consider defining these script IDs as constants or class variables to enhance maintainability and readability.
Line range hint
1284-1410: Correct return type annotation inconstruct_bill_lineitemsThe method
construct_bill_lineitemsis annotated to returnList[Dict], but it actually returns a tuple(expense_list, item_list). Please update the return type annotation to accurately reflect the returned tuple, such as-> Tuple[List[Dict], List[Dict]].
Update test calls to include the
general_mappingparameter correctlyThe verification reveals inconsistencies in how the method is called:
- In
apps/netsuite/tasks.py: The call is correct, includinggeneral_mappings- In
tests/test_netsuite/test_connector.py: Two test calls are passing an empty dictionary{}which may not be the correct implementation forgeneral_mapping🔗 Analysis chain
Line range hint
1628-1674: Verify all calls topost_credit_card_chargeinclude the newgeneral_mappingparameterThe
post_credit_card_chargemethod now includes thegeneral_mappingparameter. Ensure all calls to this method are updated accordingly.Run the following script to check method calls:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `post_credit_card_charge` and verify the inclusion of `general_mapping`. rg --py --pcre2 "post_credit_card_charge\(([^)]*)\)" -t py -A 1 | grep -n "post_credit_card_charge"Length of output: 506
Script:
#!/bin/bash # Search for calls to post_credit_card_charge method rg "post_credit_card_charge\(" -t py -A 2Length of output: 1298
📜 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 due to trivial changes (1)
- apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_fyle/conftest.py
🔇 Additional comments (42)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
5-5: Verify the PostgreSQL version downgrade
The PostgreSQL version has been downgraded from 15.9 to 15.7. Please confirm if this aligns with the target environment's requirements.
962-963: Verify data migration for new NOT NULL column
The split_expense_grouping column is added with a NOT NULL constraint. Ensure that a data migration exists to populate this column for existing records.
✅ Verification successful
Migration includes default value for NOT NULL constraint
The migration safely handles the NOT NULL constraint by providing a default value through the get_default_split_expense_grouping function, ensuring existing records will have a valid value. The field also has proper choices defined: 'SINGLE_LINE_ITEM' and 'MULTIPLE_LINE_ITEM'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related migrations that handle data population
# Search for migrations related to split_expense_grouping
fd -e py -x rg -l 'split_expense_grouping' {} apps/fyle/migrations/
Length of output: 19267
Script:
#!/bin/bash
# Let's check the content of the migration file to verify if it handles data population
cat apps/fyle/migrations/0035_support_split_expense_grouping.py
Length of output: 950
1445-1446: Verify data migration for new NOT NULL column
The is_tax_balancing_enabled column is added with a NOT NULL constraint. Ensure that a data migration exists to populate this column for existing records.
✅ Verification successful
Let me check the content of the migration file to verify if it properly handles data population for existing records.
Migration safely handles NOT NULL constraint with default value
The migration properly handles the data population for existing records by setting a default value of False for the is_tax_balancing_enabled column. This ensures that all existing records will automatically receive this default value when the column is added, satisfying the NOT NULL constraint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related migrations that handle data population
# Search for migrations related to is_tax_balancing_enabled
fd -e py -x rg -l 'is_tax_balancing_enabled' {} apps/mappings/migrations/
Length of output: 23240
Script:
#!/bin/bash
# Check the content of the migration file to see how it handles existing records
cat apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py
Length of output: 531
7993-7997: Verify migration dependencies
The migrations appear to be in the correct order. Please verify that:
- Migration 0035_support_split_expense_grouping depends on 0034_expense_is_posted_at_null
- Migration 0015_generalmapping_is_tax_balancing_enabled has no conflicts with existing migrations
✅ Verification successful
Based on the output, I can now generate the final response since I have verified the migration dependencies:
Migration dependencies are correctly ordered
The migration dependencies have been verified:
0035_support_split_expense_groupingcorrectly depends on0034_expense_is_posted_at_nullin the fyle app0015_generalmapping_is_tax_balancing_enableddepends on0014_auto_20240417_0807in the mappings app with no conflicts in the dependency chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check migration dependencies
# Search for migration dependencies
fd -e py -x rg -A 5 'dependencies = \[' {} apps/fyle/migrations/ apps/mappings/migrations/
Length of output: 113354
tests/test_fyle/fixtures.py (3)
1-1: LGTM!
The datetime import is appropriately added and necessary for creating test timestamps.
1011-1011: LGTM!
The split expense grouping configuration is consistently set to "MULTIPLE_LINE_ITEM" across different configuration objects, which aligns with the test data structure.
Also applies to: 1115-1116
249-249: LGTM! Verify test coverage for different bank transaction scenarios.
The additions of bank_transaction_id and is_posted_at_null fields provide good test data for split expense grouping. The same bank transaction ID across multiple expenses allows testing grouping logic, while varying posted statuses enable testing different expense states.
Let's verify that we have test coverage for all scenarios:
Also applies to: 287-287, 291-291, 329-329, 333-333, 371-371, 391-392, 399-400, 622-622, 664-665, 866-867, 909-910, 985-986
✅ Verification successful
Test coverage appears sufficient for split expense scenarios
Based on the test implementations found, there is good coverage for split expense grouping scenarios including:
- Expenses with no bank transaction ID
- Expenses with same and different bank transaction IDs
- Multiple pairs of expenses with same bank transaction IDs
While we couldn't directly verify the posted status variations in the test file, the test structure shows comprehensive coverage of the core split expense grouping functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if we have test cases covering different combinations of bank_transaction_id and posted status
# Search for test methods related to split expenses
rg -l "def test.*split.*expense" tests/
Length of output: 72
Script:
#!/bin/bash
# Let's examine the actual test implementations for split expenses
rg -A 10 "def test.*split.*expense" tests/test_fyle/test_models.py
Length of output: 1577
Script:
#!/bin/bash
# Let's check if there are test cases covering posted status variations
rg -B 2 -A 10 "is_posted_at_null" tests/test_fyle/test_models.py
Length of output: 64
Script:
#!/bin/bash
# Let's check the complete test file content to understand the test coverage
rg -A 30 "class.*Test.*Split" tests/test_fyle/test_models.py
Length of output: 62
apps/fyle/migrations/0035_support_split_expense_grouping.py (2)
19-23: 🛠️ Refactor suggestion
Review the split_expense_grouping field implementation
Several aspects of the split_expense_grouping field implementation could be improved:
- Using a function for default values in migrations can be risky as the function might change over time, affecting the reproducibility of the migration
- The field length (100) seems excessive for the current choices which are much shorter
- The choices could be defined more efficiently
Consider applying these modifications:
migrations.AddField(
model_name='expensegroupsettings',
name='split_expense_grouping',
- field=models.CharField(choices=[('SINGLE_LINE_ITEM', 'SINGLE_LINE_ITEM'), ('MULTIPLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM')], default=apps.fyle.models.get_default_split_expense_grouping, help_text='specify line items for split expenses grouping', max_length=100),
+ field=models.CharField(
+ choices=[
+ ('SINGLE_LINE_ITEM', 'Single Line Item'),
+ ('MULTIPLE_LINE_ITEM', 'Multiple Line Items')
+ ],
+ default='SINGLE_LINE_ITEM', # Hardcode the default value
+ help_text='specify line items for split expenses grouping',
+ max_length=20 # Reduced to a more appropriate length
+ ),
),Let's verify if the default value function is used elsewhere in the codebase:
13-24: Consider adding a data migration for existing records
The migration adds new fields but doesn't include a data migration to set initial values for existing records. While both fields are optional, it might be beneficial to include a data migration to ensure consistent state across all records.
Consider adding a data migration operation to:
- Set default values for
split_expense_groupingon existing ExpenseGroupSettings records - Set
bank_transaction_idvalues if they can be derived from existing data
Let's check if there are existing records that might need migration:
apps/mappings/models.py (2)
71-71: LGTM! Verify the integration with tax balancing logic.
The new boolean field is well-defined with appropriate default value and help text.
Let's verify how this field is used in the codebase:
✅ Verification successful
Field is properly integrated with tax balancing logic
The field is well integrated into the codebase:
- Used in
NetSuiteConnectorto control tax amount recalculation logic - Covered by comprehensive test cases for different transaction types:
- Expense reports
- Bill items
- Journal entries
- Credit card charges
- Has corresponding migration file and database schema updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to is_tax_balancing_enabled to ensure proper integration
rg "is_tax_balancing_enabled" --type python
Length of output: 74
Script:
#!/bin/bash
# Search for references to is_tax_balancing_enabled without file type restriction
rg "is_tax_balancing_enabled"
# Also search for any tax balancing related code
rg "tax_balancing" -A 2
Length of output: 7794
73-73:
Breaking Change: Adding related_name to existing relationship
Adding related_name='general_mappings' to an existing OneToOne field is a breaking change that could affect existing code using reverse lookups. Ensure all reverse relationship references are updated accordingly.
Let's check for potential impacts:
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (1)
29-30: LGTM! The new field is consistently added.
The split_expense_grouping field is correctly added with the value 'MULTIPLE_LINE_ITEM' in both the export_settings and response sections.
Also applies to: 78-79
apps/workspaces/apis/export_settings/serializers.py (1)
88-88: Consider the impact of adding a required field without defaults
Adding a required field without defaults could break existing API clients and affect database migrations. Consider:
- Adding field validation using
choicesparameter to restrict valid values - Providing a default value to maintain backward compatibility
- Documenting valid values in the field's docstring
Let's check if there are any existing values or validation patterns:
tests/test_netsuite/fixtures.py (4)
5-31: LGTM! Well-structured test data for tax details
The new tax_list_detail structure provides comprehensive test coverage for tax scenarios with properly nested data matching NetSuite's format.
994-999: LGTM! Consistent taxCode structure
The taxCode field structure follows NetSuite's data model with all required fields (externalId, internalId, name, type).
1205-1205: LGTM! Improved custom field type handling
Adding explicit type field for custom fields enhances test coverage for custom field scenarios.
Also applies to: 1208-1208
1215-1217: LGTM! Standardized taxCode structure
The taxCode structure maintains consistency with the rest of the test data and NetSuite's data model.
apps/fyle/models.py (7)
28-29: Include 'bank_transaction_id' in ALLOWED_FIELDS
Adding 'bank_transaction_id' to the ALLOWED_FIELDS list ensures that this field can be used for grouping and filtering expenses. This is appropriate and aligns with the new functionality introduced.
76-77: Define SPLIT_EXPENSE_GROUPING choices
Defining the SPLIT_EXPENSE_GROUPING tuple with ('SINGLE_LINE_ITEM', 'SINGLE_LINE_ITEM') and ('MULTIPLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM') provides clear options for split expense grouping settings. This addition is well-defined and follows the Django model field choices convention.
109-109: Add bank_transaction_id field to Expense model
Introducing the bank_transaction_id field to the Expense model allows for storing bank transaction identifiers associated with expenses. The field is correctly defined as a CharField, is nullable, and includes a helpful help_text.
180-180: Include bank_transaction_id in expense defaults
Including 'bank_transaction_id' in the defaults dictionary within create_expense_objects ensures that the bank_transaction_id is correctly set when creating or updating expense objects. This is necessary for the new field to be populated in the database.
215-217: Define default for split_expense_grouping
The function get_default_split_expense_grouping returns 'MULTIPLE_LINE_ITEM' as the default value for split_expense_grouping. This default aligns with the expected behavior and provides a sensible fallback.
243-247: Add split_expense_grouping field to ExpenseGroupSettings
Adding the split_expense_grouping field to the ExpenseGroupSettings model allows users to specify how split expenses should be grouped. The field is properly defined with a choices parameter and a default value, adhering to best practices.
327-328: Update update_expense_group_settings to include split_expense_grouping
Including split_expense_grouping in the defaults dictionary within update_expense_group_settings ensures that this setting is saved when expense group settings are updated. This change integrates the new field seamlessly into the settings update process.
tests/test_netsuite/test_connector.py (13)
9-9: Importing GeneralMapping
The import statement for GeneralMapping is correctly added and is necessary for accessing the model in the tests.
22-22: Retrieving GeneralMapping instance
The code correctly retrieves the GeneralMapping instance for workspace_id=1, ensuring that the general_mapping object is available for the test.
26-26: Updating method call with general_mapping parameter
The test method _NetSuiteConnector__construct_expense_report is updated to include the general_mapping parameter, aligning with the updated method signature.
82-85: Updating test_construct_bill_account_based with general_mapping
The test now retrieves general_mapping and includes it in the call to _NetSuiteConnector__construct_bill, ensuring consistency with the updated method.
96-99: Updating test_construct_bill_item_based with general_mapping
The general_mapping object is correctly retrieved and passed to the method, matching the new parameter requirements.
108-111: Updating test_construct_bill_item_and_account_based with general_mapping
Including general_mapping in the method call aligns with the updated method signature.
853-853: Retrieving GeneralMapping in test_post_bill_exception
The addition of general_mapping retrieval ensures that the test aligns with updated method requirements.
863-863: Updating post_bill method call with general_mapping
Passing general_mapping to post_bill aligns with the updated method signature and ensures the test remains valid.
871-871: Retrieving GeneralMapping in test_post_expense_report_exception
Including general_mapping retrieval prepares the test for the updated method signature.
881-881: Updating post_expense_report method call with general_mapping
The method call now correctly includes general_mapping, aligning with the updated requirements.
889-889: Retrieving GeneralMapping in test_post_journal_entry_exception
Retrieving general_mapping ensures it is available for the subsequent method call.
901-901: Updating post_journal_entry method call with general_mapping
Including general_mapping in the method call maintains consistency with the updated method signature.
1022-1070: Adding test test_constructs_tax_details_list_for_multiple_items
The new test effectively validates the construction of tax details for multiple items, ensuring correct handling in construct_tax_details_list. The use of mocks and assertions is appropriate.
apps/netsuite/tasks.py (1)
490-490: Verify inclusion of general_mappings in post_bill function
In line 490, the post_bill function is called with general_mappings. Confirm that general_mappings is required by the post_bill function and that it is being passed correctly.
apps/netsuite/connector.py (4)
Line range hint 1284-1410: Verify all calls to construct_bill_lineitems include the new general_mapping parameter
The construct_bill_lineitems method now requires a general_mapping parameter. Ensure that all calls to this method throughout the codebase have been updated to include this new parameter to prevent TypeError exceptions.
Run the following script to verify method calls:
✅ Verification successful
All calls to construct_bill_lineitems include the new general_mapping parameter
The codebase contains only one call to construct_bill_lineitems which correctly includes all required parameters, including the new general_mapping parameter (passed as general_mappings). No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `construct_bill_lineitems` and check if the new parameter is included.
rg --py --pcre2 "construct_bill_lineitems\(([^)]*)\)" -t py -A 1 | grep -n "construct_bill_lineitems"
Length of output: 508
Script:
#!/bin/bash
# Description: Find all calls to construct_bill_lineitems method
rg "construct_bill_lineitems\(" -t py -A 2
Length of output: 501
Line range hint 1549-1567: Verify all calls to post_bill include the new general_mappings parameter
The post_bill method signature now includes the general_mappings parameter. Please ensure that all invocations of this method have been updated to include this parameter.
Run the following script to find and verify method calls:
✅ Verification successful
Let me try a simpler search pattern to find the method calls first. This will help us identify where the method is being called and then we can analyze the parameters being passed.
All calls to post_bill include the required general_mappings parameter
The verification shows that both calls to post_bill in the codebase correctly include the general_mappings parameter:
tests/test_netsuite/test_connector.py:post_bill(bill_transaction, bill_transaction_lineitems, general_mapping)apps/netsuite/tasks.py:post_bill(bill_object, bill_lineitems_objects, general_mappings)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `post_bill` and check if the new parameter is included.
rg --py --pcre2 "post_bill\(([^)]*)\)" -t py -A 1 | grep -n "post_bill"
Length of output: 478
Script:
#!/bin/bash
# Search for post_bill method calls in Python files
rg "post_bill\(" -t py -A 2
Length of output: 757
Line range hint 2251-2272: Verify all calls to post_journal_entry include the new general_mapping parameter
Ensure that all calls to the post_journal_entry method include the updated signature with the general_mapping parameter.
Run the following script to find and verify method calls:
✅ Verification successful
Let me try a different approach to find the method calls. The previous command had an incorrect flag.
All calls to post_journal_entry include the required general_mapping parameter
Based on the search results, all calls to post_journal_entry method include the required general_mapping parameter:
apps/netsuite/tasks.py: Calls withgeneral_mappingtests/test_netsuite/test_connector.py: Calls withgeneral_mapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `post_journal_entry` and verify the inclusion of `general_mapping`.
rg --py --pcre2 "post_journal_entry\(([^)]*)\)" -t py -A 1 | grep -n "post_journal_entry"
Length of output: 496
Script:
#!/bin/bash
# Search for calls to post_journal_entry method
rg "post_journal_entry\(" -t py -A 2
Length of output: 1129
Line range hint 1988-2080: Verify all calls to construct_journal_entry_lineitems include the new general_mapping parameter
The method construct_journal_entry_lineitems now requires a general_mapping parameter. Please ensure all its invocations include this parameter.
Run the following script to verify method calls:
✅ Verification successful
Let me try a simpler pattern to find the method calls first. If we find any matches, we can analyze them further to verify the parameters.
I need to see more context around the debit_line call to verify if general_mapping parameter is properly passed. The credit_line call looks correct, but we need to check the complete context of the debit_line call.
All calls to construct_journal_entry_lineitems properly include the general_mapping parameter
Both invocations of the method correctly pass the required general_mapping parameter:
credit_line = self.construct_journal_entry_lineitems(journal_entry_lineitems, credit='Credit', org_id=org_id, general_mapping=general_mapping)debit_line = self.construct_journal_entry_lineitems(journal_entry_lineitems, debit='Debit', attachment_links={}, cluster_domain=cluster_domain, org_id=org_id, general_mapping=general_mapping)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `construct_journal_entry_lineitems` and check for the new parameter.
rg --py --pcre2 "construct_journal_entry_lineitems\(([^)]*)\)" -t py -A 1 | grep -n "construct_journal_entry_lineitems"
Length of output: 526
Script:
#!/bin/bash
# Search for calls to construct_journal_entry_lineitems with better pattern matching
rg "construct_journal_entry_lineitems\(" -t py -A 2
Length of output: 908
Script:
#!/bin/bash
# Get more context around the debit_line call to see if general_mapping is passed
rg "construct_journal_entry_lineitems\(" -t py -B 2 -A 5
Length of output: 1506
| rollback; | ||
| begin; |
There was a problem hiding this comment.
Add COMMIT statement to complete the transaction.
The transaction is properly started with rollback and begin, but missing a COMMIT statement at the end. This could leave the transaction open.
Apply this diff:
rollback;
begin;
UPDATE expense_group_settings
SET split_expense_grouping = 'SINGLE_LINE_ITEM';
+
+COMMIT;📝 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.
| rollback; | |
| begin; | |
| rollback; | |
| begin; | |
| UPDATE expense_group_settings | |
| SET split_expense_grouping = 'SINGLE_LINE_ITEM'; | |
| COMMIT; |
| UPDATE expense_group_settings | ||
| SET split_expense_grouping = 'SINGLE_LINE_ITEM'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding safety measures to the UPDATE statement.
The current UPDATE affects all rows without any safety measures. Consider:
- Adding a WHERE clause to target only rows needing updates
- Adding error handling
- Adding logging of affected rows count
Here's a safer version:
-UPDATE expense_group_settings
-SET split_expense_grouping = 'SINGLE_LINE_ITEM';
+DO $$
+DECLARE
+ affected_rows INTEGER;
+BEGIN
+ WITH updated AS (
+ UPDATE expense_group_settings
+ SET split_expense_grouping = 'SINGLE_LINE_ITEM'
+ WHERE split_expense_grouping IS NULL
+ RETURNING *
+ )
+ SELECT COUNT(*) INTO affected_rows FROM updated;
+
+ RAISE NOTICE 'Updated % rows in expense_group_settings', affected_rows;
+
+ IF affected_rows = 0 THEN
+ RAISE NOTICE 'No rows needed updating';
+ END IF;
+END $$;📝 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.
| UPDATE expense_group_settings | |
| SET split_expense_grouping = 'SINGLE_LINE_ITEM'; | |
| DO $$ | |
| DECLARE | |
| affected_rows INTEGER; | |
| BEGIN | |
| WITH updated AS ( | |
| UPDATE expense_group_settings | |
| SET split_expense_grouping = 'SINGLE_LINE_ITEM' | |
| WHERE split_expense_grouping IS NULL | |
| RETURNING * | |
| ) | |
| SELECT COUNT(*) INTO affected_rows FROM updated; | |
| RAISE NOTICE 'Updated % rows in expense_group_settings', affected_rows; | |
| IF affected_rows = 0 THEN | |
| RAISE NOTICE 'No rows needed updating'; | |
| END IF; | |
| END $$; |
| 'ccc_expense_state': 'PAID', | ||
| 'split_expense_grouping': 'MULTIPLE_LINE_ITEM' |
There was a problem hiding this comment.
💡 Codebase verification
The missing values test cases need to be updated
Based on the serializer definition and test fixtures:
- The
split_expense_groupingfield is marked as required (required=True) in the serializer - The field is missing in the following test cases that test missing values scenarios:
export_settings_missing_values_configurations.expense_group_settingsexport_settings_missing_values_expense_group_settings(completely empty settings)export_settings_missing_values_general_mappings.expense_group_settings
🔗 Analysis chain
Verify if missing values test cases need updating.
The new split_expense_grouping field is missing in the following test sections:
export_settings_missing_values_configurations.expense_group_settingsexport_settings_missing_values_general_mappings.expense_group_settings
Please verify if these sections should include the new field for comprehensive testing.
Also applies to: 78-79
Let me check the missing values test cases to verify if they need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if split_expense_grouping is a required field in the serializer
# and if missing values test cases should be updated accordingly
# Search for the serializer definition and field requirements
rg -A 10 "class.*ExpenseGroupSettingsSerializer"
Length of output: 2594
Script:
#!/bin/bash
# Search for the missing values test cases in the test file
rg -B 5 -A 10 "export_settings_missing_values" tests/test_workspaces/test_apis/test_export_settings/fixtures.py
Length of output: 1826
apps/netsuite/tasks.py
Outdated
| @@ -633,7 +634,7 @@ def create_expense_report(expense_group, task_log_id, last_export): | |||
| ) | |||
|
|
|||
| created_expense_report = netsuite_connection.post_expense_report( | |||
| expense_report_object, expense_report_lineitems_objects | |||
| expense_report_object, expense_report_lineitems_objects, general_mapping | |||
There was a problem hiding this comment.
Correct variable name in post_expense_report function call
In line 637, general_mapping is passed to post_expense_report. Ensure that this variable is correctly named and defined. If general_mappings is intended, update the variable name.
Apply this diff to use the consistent variable name:
- created_expense_report = netsuite_connection.post_expense_report(
- expense_report_object, expense_report_lineitems_objects, general_mapping
+ created_expense_report = netsuite_connection.post_expense_report(
+ expense_report_object, expense_report_lineitems_objects, general_mappings
)Committable suggestion skipped: line range outside the PR's diff.
apps/netsuite/tasks.py
Outdated
| @@ -567,7 +567,7 @@ | |||
| attachment_links[expense.expense_id] = attachment_link | |||
|
|
|||
| created_credit_card_charge = netsuite_connection.post_credit_card_charge( | |||
| credit_card_charge_object, credit_card_charge_lineitems_object, attachment_links, refund | |||
| credit_card_charge_object, credit_card_charge_lineitems_object, general_mappings, attachment_links, refund | |||
There was a problem hiding this comment.
Ensure general_mappings is defined and passed correctly
At line 570, general_mappings is passed to post_credit_card_charge, but it may not be defined in the current scope. This could cause a NameError.
Verify that general_mappings is defined before this line or adjust the variable name if necessary. If you intended to use general_mapping, apply this diff:
- created_credit_card_charge = netsuite_connection.post_credit_card_charge(
- credit_card_charge_object, credit_card_charge_lineitems_object, general_mappings, attachment_links, refund
+ created_credit_card_charge = netsuite_connection.post_credit_card_charge(
+ credit_card_charge_object, credit_card_charge_lineitems_object, general_mapping, attachment_links, refund
)Committable suggestion skipped: line range outside the PR's diff.
apps/netsuite/tasks.py
Outdated
| @@ -337,11 +337,11 @@ | |||
| elif task_log.type == 'CREATING_BILL': | |||
| construct_lines = getattr(netsuite_connection, func) | |||
| # calling the target construct payload function | |||
| expense_list, item_list = construct_lines(export_line_items, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details) | |||
| expense_list, item_list = construct_lines(export_line_items, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details, general_mappings) | |||
There was a problem hiding this comment.
Correct the parameter order and remove duplicate general_mappings
At line 340, the construct_lines function is called with general_mappings passed twice, and the parameter order differs from other calls. This could lead to unexpected behavior or errors.
Apply this diff to correct the parameter order and remove the duplicate general_mappings:
- expense_list, item_list = construct_lines(export_line_items, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details, general_mappings)
+ expense_list, item_list = construct_lines(export_line_items, general_mappings, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details)📝 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_list, item_list = construct_lines(export_line_items, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details, general_mappings) | |
| expense_list, item_list = construct_lines(export_line_items, general_mappings, expense_id_receipt_url_map, cluster_domain, workspace.fyle_org_id, general_mappings.override_tax_details) |
apps/netsuite/tasks.py
Outdated
| @@ -698,7 +700,7 @@ | |||
| ) | |||
|
|
|||
| created_journal_entry = netsuite_connection.post_journal_entry( | |||
| journal_entry_object, journal_entry_lineitems_objects, configuration | |||
| journal_entry_object, journal_entry_lineitems_objects, configuration, general_mapping | |||
There was a problem hiding this comment.
Correct parameter in post_journal_entry function call
In line 703, general_mapping is passed to post_journal_entry, but it might not be the intended variable. Verify that general_mapping is defined and correctly represents the required data.
Apply this diff if general_mappings is the correct variable:
- created_journal_entry = netsuite_connection.post_journal_entry(
- journal_entry_object, journal_entry_lineitems_objects, configuration, general_mapping
+ created_journal_entry = netsuite_connection.post_journal_entry(
+ journal_entry_object, journal_entry_lineitems_objects, configuration, general_mappings
)Committable suggestion skipped: line range outside the PR's diff.
apps/netsuite/connector.py
Outdated
|
|
||
| def handle_taxed_line_items(self, base_line, line, workspace_id, export_module, general_mapping: GeneralMapping): | ||
| """ | ||
| Create bill line items | ||
| :return: constructed line items | ||
| Handle line items where tax is applied or modified by the user. | ||
| :param base_line: The base line item template that will be modified. | ||
| :param line: The original line with tax and amount information. | ||
| :param is_credit_card_charge: Boolean flag to differentiate between credit card charges and other transactions. | ||
| :return: List of lines (taxed and/or untaxed). |
There was a problem hiding this comment.
Fix docstring to match method signature in handle_taxed_line_items
The docstring of the handle_taxed_line_items method mentions a parameter is_credit_card_charge which is not present in the method signature. Please update the docstring to accurately reflect the current parameters.
JustARatherRidiculouslyLongUsername
force-pushed
the
ut-split-expense-grouping
branch
from
a54d312 to
6a50afe
Compare
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
…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
JustARatherRidiculouslyLongUsername
merged commit d545449
into
split-expense-grouping
|
* 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/86cx0vh5k
Summary by CodeRabbit
New Features
Tests