feat: support GET and PUT for split_expense_grouping in export settings
#672
Conversation
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request involve the addition of new fields and modifications to existing methods across several files. Notably, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 16 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
force-pushed
the
split-expenses-serializer
branch
from
482a4f8 to
cd0b8e6
Compare
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-db-changes
branch
from
a41a418 to
b24e6c6
Compare
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-serializer
branch
from
cd0b8e6 to
aaba448
Compare
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-db-changes
branch
from
b24e6c6 to
fd02002
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
tests/test_fyle/fixtures.py (1)
249-249: Consider using unique bank_transaction_ids for different test casesThe same
bank_transaction_idvalue ('btxnqmATEEBkJ4') is used across multiple test fixtures. While this might work for basic testing, using unique IDs would help catch potential uniqueness constraints or ID-related bugs.Consider using distinct IDs like:
-'bank_transaction_id': 'btxnqmATEEBkJ4', +'bank_transaction_id': 'btxn_1234', // For expense ID 1234 +'bank_transaction_id': 'btxn_1235', // For expense ID 1235 +'bank_transaction_id': 'btxn_1236', // For expense ID 1236Also applies to: 291-291, 333-333, 866-866, 909-909
tests/test_fyle/conftest.py (3)
Line range hint
34-44: Remove duplicate 'report_id' field from corporate_credit_card_expense_group_fieldsThe 'report_id' field appears twice in the list, which could lead to confusion and potential issues.
Apply this diff to remove the duplicate:
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' ]
Line range hint
34-44: Consider adding validation for configuration valuesThe fixture sets hardcoded values for configurations without validation:
- 'CREDIT CARD CHARGE' for corporate_credit_card_expenses_object
- 'SINGLE_LINE_ITEM' for split_expense_grouping
Consider adding constants or enums to validate these values and make the test more maintainable.
Example implementation:
from enum import Enum class ExpenseObject(str, Enum): CREDIT_CARD_CHARGE = 'CREDIT CARD CHARGE' BANK_TRANSACTION = 'BANK TRANSACTION' class ExpenseGrouping(str, Enum): SINGLE_LINE_ITEM = 'SINGLE_LINE_ITEM' MULTIPLE_LINE_ITEMS = 'MULTIPLE_LINE_ITEMS' # Then in the fixture: general_settings.corporate_credit_card_expenses_object = ExpenseObject.CREDIT_CARD_CHARGE.value expense_group_settings.split_expense_grouping = ExpenseGrouping.SINGLE_LINE_ITEM.value
Line range hint
34-44: Consider adding assertions to verify configuration changesThe fixture modifies configuration but doesn't verify if the changes were applied correctly. Consider adding assertions to ensure the configuration is updated as expected.
Example implementation:
def _update_config_for_split_expense_grouping(general_settings, expense_group_settings): general_settings.corporate_credit_card_expenses_object = 'CREDIT CARD CHARGE' general_settings.save() # Verify general settings assert general_settings.refresh_from_db() is None, "Failed to save general settings" assert general_settings.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' expense_group_settings.split_expense_grouping = 'SINGLE_LINE_ITEM' expense_group_settings.corporate_credit_card_expense_group_fields = [ 'expense_id', 'claim_number', 'fund_source', 'employee_email', 'report_id', 'spent_at' ] expense_group_settings.save() # Verify expense group settings assert expense_group_settings.refresh_from_db() is None, "Failed to save expense group settings" assert expense_group_settings.split_expense_grouping == 'SINGLE_LINE_ITEM' assert 'spent_at' in expense_group_settings.corporate_credit_card_expense_group_fieldstests/test_workspaces/test_apis/test_export_settings/fixtures.py (1)
Test cases for split_expense_grouping field should be added to missing value scenarios
Based on the search results,
split_expense_groupingis a required field in the serializer (allow_null=False, allow_blank=False, required=True). The field should be included in all test scenarios to ensure proper validation coverage.
- Add
split_expense_groupingfield toexport_settings_missing_values_configurations.expense_group_settings- Add
split_expense_groupingfield toexport_settings_missing_values_general_mappings.expense_group_settings🔗 Analysis chain
Line range hint
1-200: Consider adding test cases for split_expense_groupingThe field
split_expense_groupingis missing in some test scenarios:
- Not present in
export_settings_missing_values_configurations.expense_group_settings- Not present in
export_settings_missing_values_general_mappings.expense_group_settingsIf this is a required field as indicated by the serializer, we should either:
- Add the field to all test scenarios, or
- Document why certain test scenarios exclude this field (e.g., if testing validation errors)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the field is required in the serializer ast-grep --pattern 'split_expense_grouping = serializers.CharField( $$$ required=$_, $$$ )'Length of output: 99
Script:
#!/bin/bash # Let's search for the serializer definition with a broader pattern rg -A 5 "split_expense_grouping" --type pyLength of output: 9817
tests/test_fyle/test_models.py (4)
166-166: LGTM! Consider these improvements for better test maintainability.The addition of
.distinct()is correct and necessary to prevent duplicate counting in the query results. However, consider these improvements:
- Add detailed docstrings explaining both test scenarios (SINGLE_LINE_ITEM and MULTIPLE_LINE_ITEM)
- Use descriptive variable names (e.g.,
expense_groupsinstead ofgroups)- Define test data constants (e.g.,
EXPECTED_SINGLE_LINE_GROUPS = 2)
204-204: LGTM! Consider restructuring for better readability.The
.distinct()addition is correct. To improve test clarity:
- Extract test data setup into helper methods
- Use more descriptive assertion messages explaining the expected grouping behavior
- Structure the test with clear setup/exercise/verify sections
Example structure:
def test_split_expense_grouping_with_same_and_different_ids(db, update_config_for_split_expense_grouping): """Test expense grouping with mixed bank transaction IDs. Scenario: - Two expenses sharing bank_transaction_id='sample_1' - One expense with bank_transaction_id='sample_2' """ # SETUP workspace_id = 1 test_expenses = create_test_expenses_with_mixed_transaction_ids() # EXERCISE groups_single_line = create_groups_with_single_line_setting(test_expenses) groups_multiple_line = create_groups_with_multiple_line_setting(test_expenses) # VERIFY assert_correct_grouping_for_single_line(groups_single_line) assert_correct_grouping_for_multiple_line(groups_multiple_line)
243-245: Fix formatting and consider improvements for clarity.The
.distinct()addition is correct, but there are some issues to address:
- Fix formatting: Add space around operator in
old_count== 2- Consider renaming test to better describe the scenario (e.g.,
test_split_expense_grouping_with_paired_transaction_ids)- Apply similar improvements as suggested in previous tests
- 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}'
Line range hint
140-245: Consider using pytest parameterization for better test organization.The three test functions for split expense grouping share similar patterns and setup code. Consider refactoring them using pytest's parameterization feature to reduce duplication and improve maintainability.
Example approach:
@pytest.mark.parametrize('test_scenario', [ { 'name': 'no_bank_transaction_id', 'expenses': [ {'bank_transaction_id': None}, {'bank_transaction_id': None} ], 'expected_single_line_groups': 2, 'expected_multiple_line_groups': 2 }, # Add other scenarios... ]) def test_split_expense_grouping(db, update_config_for_split_expense_grouping, test_scenario): """Test expense grouping with different bank transaction ID scenarios.""" # Common setup and verification logic hereapps/fyle/models.py (2)
75-76: Add docstring to explain the purpose of split expense grouping choices.Consider adding documentation to explain when to use each option and their implications.
SPLIT_EXPENSE_GROUPING = (('SINGLE_LINE_ITEM', 'SINGLE_LINE_ITEM'), ('MULTIPLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM')) + +""" +Choices for handling split expenses in expense groups: +- SINGLE_LINE_ITEM: Consolidates split expenses into a single line item +- MULTIPLE_LINE_ITEM: Maintains separate line items for split expenses +"""Also applies to: 214-216
75-76: Consider documenting performance implications of grouping choices.The choice between
SINGLE_LINE_ITEMandMULTIPLE_LINE_ITEMcould have significant implications:
- Database query performance
- Memory usage when processing large numbers of split expenses
- Impact on downstream systems (e.g., NetSuite) when processing grouped vs. individual line items
Consider adding documentation about these trade-offs to help users make informed decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/fyle/migrations/0035_support_split_expense_grouping.py(1 hunks)apps/fyle/models.py(6 hunks)apps/workspaces/apis/export_settings/serializers.py(2 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(7 hunks)tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/fixtures.py(7 hunks)tests/test_fyle/test_models.py(3 hunks)tests/test_workspaces/test_apis/test_export_settings/fixtures.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspaces/apis/export_settings/serializers.py
🔇 Additional comments (12)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (5)
1153-1154: LGTM: Appropriate column definition for bank_transaction_id
The bank_transaction_id column is appropriately defined as an optional VARCHAR(255), which is suitable for transaction identifiers.
7997-7997: LGTM: Migration sequence properly updated
The migration sequence is correctly incremented and the migration name follows the convention.
962-963: Verify data migration strategy for NOT NULL constraint
The new split_expense_grouping column is added with a NOT NULL constraint. Ensure there's a proper data migration strategy to populate this field for existing records.
✅ Verification successful
Migration strategy for NOT NULL constraint is properly handled
The migration file shows that the split_expense_grouping column is added with a default value provided by apps.fyle.models.get_default_split_expense_grouping function, which ensures existing records will have a valid value. The column also has predefined choices ('SINGLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM'), making it a safe schema change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related migration files that might handle data population
fd -e py | xargs grep -l "split_expense_grouping"
Length of output: 333
Script:
#!/bin/bash
# Check the migration file content to verify the data migration strategy
cat apps/fyle/migrations/0035_support_split_expense_grouping.py
Length of output: 950
5-5: Verify the PostgreSQL version downgrade
The PostgreSQL version has been downgraded from 15.9 to 15.7. Please confirm if this downgrade was intentional and ensure compatibility with your deployment environment.
11584-11587: Verify if 'MULTIPLE_LINE_ITEM' is the only valid value
All inserted records use 'MULTIPLE_LINE_ITEM' for split_expense_grouping. Please confirm if this is the only valid value or if other values are supported.
tests/test_fyle/fixtures.py (1)
1011-1011: Verify the allowed values for split_expense_grouping
The fixtures use 'MULTIPLE_LINE_ITEM' as the value for split_expense_grouping. To ensure robust testing:
- Consider adding test cases for other valid values
- Add test cases to verify handling of invalid values
Let's check the model definition and any existing validation:
Also applies to: 1116-1116
✅ Verification successful
Let me search for the choices definition to understand the allowed values:
Test coverage for split_expense_grouping is sufficient
The field split_expense_grouping only accepts two valid values as defined in the model:
- 'SINGLE_LINE_ITEM'
- 'MULTIPLE_LINE_ITEM'
Looking at the test files, there are already comprehensive test cases covering both values:
test_split_expense_grouping_with_no_bank_transaction_idtest_split_expense_grouping_with_same_and_different_idstest_split_expense_grouping_pairs_of_same_ids
Additionally, Django's model field with choices validates the input automatically, so invalid values are handled by the framework's validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for split_expense_grouping field definition and validation
rg -A 5 "split_expense_grouping" --type py
Length of output: 9817
Script:
#!/bin/bash
# Search for SPLIT_EXPENSE_GROUPING choices definition
rg "SPLIT_EXPENSE_GROUPING.*=" --type py
Length of output: 295
apps/fyle/migrations/0035_support_split_expense_grouping.py (3)
14-18: LGTM: Bank transaction ID field looks good
The field definition is appropriate for storing bank transaction IDs with sufficient length and proper nullability settings.
21-22: Verify choice values match business requirements
The choices for split_expense_grouping are limited to:
- SINGLE_LINE_ITEM
- MULTIPLE_LINE_ITEM
Please confirm these values align with the business requirements from the Clickup task.
Consider documenting the meaning and use cases of each choice value in the help text for better maintainability.
19-23: Verify data migration strategy for non-nullable field
The split_expense_grouping field is being added as non-nullable with a default value. Please ensure:
- The default function
get_default_split_expense_groupingreturns a valid choice - Existing records will be properly handled during migration
✅ Verification successful
Data migration strategy for non-nullable field is safe
The verification confirms:
get_default_split_expense_grouping()returns 'MULTIPLE_LINE_ITEM' which is a valid choice defined inSPLIT_EXPENSE_GROUPINGtuple- The field is properly handled during migration because:
- Default value function is correctly implemented
- The choice is valid and matches the model's choices
- Test fixtures show consistent usage of 'MULTIPLE_LINE_ITEM' across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the default function and its return value
# Check the default function implementation
ast-grep --pattern 'def get_default_split_expense_grouping():
$$$
'
# Check if there are any existing records in the table
rg -A 5 "INSERT INTO.*expense_group_settings" tests/sql_fixtures/
Length of output: 261
Script:
#!/bin/bash
# Check if MULTIPLE_LINE_ITEM is a valid choice in the model
ast-grep --pattern 'class ExpenseGroupSettings($$$):
$$$
'
# Check for any references to split_expense_grouping in tests
rg "split_expense_grouping.*MULTIPLE_LINE_ITEM" tests/
Length of output: 9300
Script:
#!/bin/bash
# Check the SPLIT_EXPENSE_GROUPING choices definition
rg "SPLIT_EXPENSE_GROUPING.*=" apps/
# Check if there are any existing records in the database from test fixtures
rg "ExpenseGroupSettings.*create" tests/
Length of output: 403
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (1)
29-30: Verify the hardcoded value for split_expense_grouping
The value 'MULTIPLE_LINE_ITEM' is hardcoded in multiple places. Let's verify if this is the only valid value or if we should test other possible values as well.
Also applies to: 78-79
✅ Verification successful
The hardcoded test value 'MULTIPLE_LINE_ITEM' is valid but test coverage could be improved
The model definition shows that split_expense_grouping accepts two valid choices:
- 'SINGLE_LINE_ITEM'
- 'MULTIPLE_LINE_ITEM'
While the test fixtures use 'MULTIPLE_LINE_ITEM', there are other test files that verify 'SINGLE_LINE_ITEM' behavior (in tests/test_fyle/conftest.py). The missing field in other test scenarios appears intentional as they likely test different configurations.
Suggestions:
- Consider adding a test case in
test_export_settings/fixtures.pythat uses 'SINGLE_LINE_ITEM' to ensure both values work correctly with export settings - The current test coverage across different files is adequate, but could be more explicit in this specific test file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the model definition to find allowed choices
ast-grep --pattern 'split_expense_grouping = models.CharField(
$$$
choices=$_,
$$$
)'
# Search for any other occurrences of this field to understand usage
rg "split_expense_grouping.*=.*" --type python
Length of output: 521
Script:
#!/bin/bash
# Let's first check the SPLIT_EXPENSE_GROUPING choices definition
rg "SPLIT_EXPENSE_GROUPING.*=" -A 5
# Check the default function
rg "get_default_split_expense_grouping"
# Check all occurrences of this field in test files
rg "split_expense_grouping" --type-list
rg "split_expense_grouping" tests/
Length of output: 4180
apps/fyle/models.py (2)
179-179: LGTM! Method updates are consistent.
The new fields are properly integrated into the existing methods:
bank_transaction_idis correctly included in the expense creation defaultssplit_expense_groupingis appropriately handled in the settings update
Also applies to: 326-327
108-108: Verify database migration impact and data handling.
The new fields look well-defined. However, since these are model changes:
- Ensure there's a corresponding migration file.
- Verify the handling of existing records when
bank_transaction_idis null. - Confirm that existing expense group settings will use the default
MULTIPLE_LINE_ITEMvalue.
Also applies to: 242-246
✅ Verification successful
Migration and data handling are properly implemented
The verification shows:
- Migration file
0035_support_split_expense_grouping.pyexists and correctly adds both fields bank_transaction_idis nullable with appropriate defaultssplit_expense_groupinghas a default value functionget_default_split_expense_grouping- Test coverage exists for various scenarios including:
- Expenses with no bank transaction ID
- Expenses with same/different transaction IDs
- Multiple pairs of split expenses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related migration file
fd --type f ".*split_expense.*\.py$" "apps/fyle/migrations"
# Check for any hardcoded references to the old model structure
rg -l "bank_transaction_id|split_expense_grouping" "tests/"
Length of output: 381
Script:
#!/bin/bash
# Check the migration file content
cat "apps/fyle/migrations/0035_support_split_expense_grouping.py"
# Check test files for handling of new fields
rg "bank_transaction_id|split_expense_grouping" "tests/test_fyle/test_models.py" -A 2
Length of output: 2944
* 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 33a5a88
into
split-expenses-db-changes
|
…680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop (cherry picked from commit c0309e8)
Clickup
https://app.clickup.com/t/86cx0vh5m
Summary by CodeRabbit
New Features
split_expense_grouping, in the Expense Group Settings, enhancing the granularity of expense tracking.bank_transaction_idfield to the Expense model for better transaction identification.Bug Fixes
Documentation