feat: implement split expense grouping functionality #675
Conversation
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce new fields to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/fyle/models.py (1)
Line range hint
378-457: Consider refactoringcreate_expense_groups_by_report_id_fund_sourcefor better maintainabilityThe
create_expense_groups_by_report_id_fund_sourcemethod has grown complex with the addition of new grouping logic. Refactoring the method by extracting portions of the code into smaller, reusable functions can enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/fyle/models.py(4 hunks)
🔇 Additional comments (2)
apps/fyle/models.py (2)
28-29: Addition of 'bank_transaction_id' to ALLOWED_FIELDS
Including 'bank_transaction_id' in ALLOWED_FIELDS ensures it can be utilized in grouping and filtering operations, which is appropriate for the new functionality.
420-452: 🛠️ Refactor suggestion
Ensure robust handling of bank_transaction_id during grouping
The logic for grouping corporate credit card expenses based on the presence of bank_transaction_id is well-implemented. However, consider the following:
- Empty Strings: Verify that expenses with an empty string for
bank_transaction_idare correctly classified. In Python, empty strings are falsy, so the current lambda functions should handle this appropriately, but explicit checks can improve clarity. - Grouping Fields Validation: After excluding
'expense_id'and'expense_number', ensure thatsplit_expense_group_fieldsdoes not become empty, which could lead to incorrect grouping.
Would you like to add unit tests to cover edge cases, such as expenses with empty bank_transaction_id values?
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 (5)
tests/test_fyle/conftest.py (2)
Line range hint
38-45: Remove duplicate 'report_id' field from corporate_credit_card_expense_group_fieldsThe list contains 'report_id' twice, which could lead to unexpected behavior in tests and might not accurately reflect the intended configuration.
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
38-45: Consider organizing fields alphabetically for better readabilityConsider sorting the fields alphabetically to maintain consistency and make it easier to scan the list.
expense_group_settings.corporate_credit_card_expense_group_fields = [ - 'expense_id', - 'claim_number', - 'fund_source', - 'employee_email', - 'report_id', - 'spent_at' + 'claim_number', + 'employee_email', + 'expense_id', + 'fund_source', + 'report_id', + 'spent_at' ]tests/test_fyle/test_models.py (3)
166-166: LGTM! Consider adding a comment explaining the distinct() usage.The addition of
.distinct()correctly prevents duplicate expense groups in the query results. Consider adding a brief comment explaining why distinct is necessary here, to help future maintainers understand the reasoning.
243-243: LGTM with minor spacing fix needed.The
.distinct()addition is correct and consistent. However, there's a minor spacing issue in the assertion line that should be fixed for consistency.- 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}'Also applies to: 245-245
166-166: Comprehensive test coverage for expense grouping scenarios.The test suite effectively covers various bank transaction ID scenarios:
- Expenses without bank transaction IDs
- Mixed cases with same and different IDs
- Paired expenses with matching IDs
The consistent use of
.distinct()across all tests ensures reliable group counting. The test structure is well-organized and maintainable.Also applies to: 204-204, 243-243
📜 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(3 hunks)
🔇 Additional comments (1)
tests/test_fyle/test_models.py (1)
204-204: LGTM! Consistent with previous test case.
The .distinct() addition maintains consistency with the previous test case and correctly handles potential duplicate groups.
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-serializer
branch
from
482a4f8 to
cd0b8e6
Compare
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-core-logic
branch
from
b9ff9c0 to
4103aaf
Compare
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-serializer
branch
from
cd0b8e6 to
aaba448
Compare
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
tests/test_fyle/fixtures.py (1)
249-249: Consider using unique bank_transaction_ids for different test expensesAll test expenses currently use the same
bank_transaction_id: 'btxnqmATEEBkJ4'. While this works for basic testing, using unique IDs would help catch potential issues with transaction uniqueness handling and improve test coverage.Consider using distinct IDs like:
-'bank_transaction_id': 'btxnqmATEEBkJ4', +'bank_transaction_id': 'btxn_1234_unique',Also applies to: 291-291, 333-333, 866-866, 909-909
apps/workspaces/apis/export_settings/serializers.py (3)
98-99: Consider organizing fields list for better readabilityThe fields list could benefit from better organization, either by:
- Grouping related fields together (e.g., all CCC fields, all reimbursable fields)
- Following alphabetical order
Line range hint
134-162: Handle default value for split_expense_grouping in update methodThe update method sets default values for other expense group settings fields when they're empty, but doesn't handle the new
split_expense_groupingfield. Since this field is required, existing records or partial updates might fail.Add default value handling:
if 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)
Line range hint
189-199: Enhance validation for required expense group settings fieldsThe validate method checks for the presence of expense_group_settings but doesn't validate required fields within it. Consider adding specific validation for
split_expense_groupingto provide clearer error messages.def validate(self, data): if not data.get('expense_group_settings'): raise serializers.ValidationError('Expense group settings are required') + expense_group_settings = data.get('expense_group_settings', {}) + if not expense_group_settings.get('split_expense_grouping'): + raise serializers.ValidationError({ + 'expense_group_settings': 'split_expense_grouping is required' + }) + if not data.get('configuration'): raise serializers.ValidationError('Configurations settings are required')apps/fyle/models.py (1)
421-458: Consider refactoring to improve code structureThe current implementation could benefit from some structural improvements:
- Extract the condition for split expense grouping into a helper method
- Reduce nesting by splitting the logic into smaller functions
- Consider consolidating the duplicate logic for handling expenses with/without bank_transaction_id
Here's a suggested refactor:
+ def _should_group_by_bank_transaction(configuration, expense_group_settings): + return ( + configuration.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' and + expense_group_settings.split_expense_grouping == 'MULTIPLE_LINE_ITEM' + ) + def _group_expenses_by_bank_transaction(expenses, group_fields, workspace_id): + expenses_without_bank_id = list(filter(lambda expense: not expense.bank_transaction_id, expenses)) + expenses_with_bank_id = list(filter(lambda expense: expense.bank_transaction_id, expenses)) + + expense_groups = [] + + if expenses_without_bank_id: + groups = _group_expenses(expenses_without_bank_id, group_fields, workspace_id) + expense_groups.extend(groups) + + if expenses_with_bank_id: + split_fields = [field for field in group_fields if field not in ('expense_id', 'expense_number')] + split_fields.append('bank_transaction_id') + groups = _group_expenses(expenses_with_bank_id, split_fields, workspace_id) + expense_groups.extend(groups) + + return expense_groups if corporate_credit_card_expenses: - if ( - configuration.corporate_credit_card_expenses_object == 'CREDIT CARD CHARGE' and - expense_group_settings.split_expense_grouping == 'MULTIPLE_LINE_ITEM' - ): - ccc_expenses_without_bank_transaction_id = list( - filter(lambda expense: not expense.bank_transaction_id, corporate_credit_card_expenses) - ) - - ccc_expenses_with_bank_transaction_id = list( - filter(lambda expense: expense.bank_transaction_id, corporate_credit_card_expenses) - ) - - if ccc_expenses_without_bank_transaction_id: - groups_without_bank_transaction_id = _group_expenses( - ccc_expenses_without_bank_transaction_id, - corporate_credit_card_expense_group_field, - workspace_id - ) - expense_groups.extend(groups_without_bank_transaction_id) - - if ccc_expenses_with_bank_transaction_id: - split_expense_group_fields = [ - field for field in corporate_credit_card_expense_group_field - if field not in ('expense_id', 'expense_number') - ] - split_expense_group_fields.append('bank_transaction_id') - - groups_with_bank_transaction_id = _group_expenses( - ccc_expenses_with_bank_transaction_id, - split_expense_group_fields, - workspace_id - ) - expense_groups.extend(groups_with_bank_transaction_id) - else: - corporate_credit_card_expense_groups = _group_expenses( - corporate_credit_card_expenses, - corporate_credit_card_expense_group_field, - workspace_id - ) - expense_groups.extend(corporate_credit_card_expense_groups) + if _should_group_by_bank_transaction(configuration, expense_group_settings): + expense_groups.extend( + _group_expenses_by_bank_transaction( + corporate_credit_card_expenses, + corporate_credit_card_expense_group_field, + workspace_id + ) + ) + else: + expense_groups.extend( + _group_expenses( + corporate_credit_card_expenses, + corporate_credit_card_expense_group_field, + workspace_id + ) + )
🛑 Comments failed to post (4)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
962-963:
⚠️ Potential issueAdd DEFAULT value for new NOT NULL column
The new
split_expense_groupingcolumn is marked as NOT NULL without a default value. This could cause issues during migration if existing rows exist. Consider adding a DEFAULT value or ensuring the migration handles existing data appropriately.- split_expense_grouping character varying(100) NOT NULL + split_expense_grouping character varying(100) NOT NULL DEFAULT 'MULTIPLE_LINE_ITEM'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.split_expense_grouping character varying(100) NOT NULL DEFAULT 'MULTIPLE_LINE_ITEM'tests/test_fyle/fixtures.py (1)
1115-1116: 🛠️ Refactor suggestion
Add test cases for different split_expense_grouping values
The test fixtures only cover the "MULTIPLE_LINE_ITEM" scenario. Consider adding test cases for other possible values to ensure comprehensive coverage of the expense grouping functionality.
Would you like me to help generate additional test cases for different grouping scenarios?
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (1)
29-30: 💡 Codebase verification
Test scenarios need updating for required field validation
The
split_expense_groupingfield is defined as required (required=True) and non-nullable (allow_null=False) in the serializer, but the missing value test scenarios don't verify this validation. The test fixtures should be updated to include test cases for this required field.Locations needing updates:
tests/test_workspaces/test_apis/test_export_settings/fixtures.py:
export_settings_missing_values_configurationsexport_settings_missing_values_expense_group_settingsexport_settings_missing_values_general_mappings🔗 Analysis chain
Verify if missing value test scenarios need updating.
The new
split_expense_groupingfield is missing from the following test scenarios:
export_settings_missing_values_configurationsexport_settings_missing_values_expense_group_settingsexport_settings_missing_values_general_mappingsPlease verify if these scenarios should test the behavior when the new field is missing.
Also applies to: 78-79
Let me gather more information about the test scenarios mentioned in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any validation or default value handling for split_expense_grouping rg -l "split_expense_grouping" | xargs rg "split_expense_grouping.*default|split_expense_grouping.*required|split_expense_grouping.*validation" # Search for any model or serializer definitions related to ExpenseGroupSettings ast-grep --pattern 'class ExpenseGroupSettings'Length of output: 348
Script:
#!/bin/bash # Search for the test scenarios mentioned in the review comment rg -A 5 "export_settings_missing_values_" tests/ # Get more context around the serializer definition rg -B 5 -A 10 "split_expense_grouping.*required" apps/workspaces/apis/export_settings/serializers.pyLength of output: 4987
apps/workspaces/apis/export_settings/serializers.py (1)
88-88: 🛠️ Refactor suggestion
Add choices validation and documentation for split_expense_grouping field
The field lacks validation for allowed values and documentation about valid choices, which are defined in the model according to the summary.
Consider adding:
- Choices validation using
validatorsorchoicesparameter- Field documentation using docstring or help_text
- split_expense_grouping = serializers.CharField(allow_null=False, allow_blank=False, required=True) + split_expense_grouping = serializers.CharField( + allow_null=False, + allow_blank=False, + required=True, + help_text='Controls how expenses are grouped. Valid values: [list valid choices here]', + validators=[validate_split_expense_grouping] # Add appropriate validator + )Committable suggestion skipped: line range outside the PR's diff.
* 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 1e79de2
into
split-expenses-serializer
…ings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
|
…680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop
* test: write unit tests and add fixtures for split expense grouping (#670) * test: write unit tests and add fixtures for split expense grouping * refactor: remove duplicate bank txn IDs * test: fix failing tests * feat: make db changes and fixture updates to support split expenses (#680) * feat: make db changes and fixture updates to support split expenses * feat: support GET and PUT for `split_expense_grouping` in export settings (#672) * feat: support GET and PUT for `split_expense_grouping` in export settings * feat: implement split expense grouping functionality (#675) * feat: implement split expense grouping functionality * feat: add script for split expense grouping (#676) * feat: add script for split expense grouping Set the default split expense grouping config to SINGLE_LINE_ITEM for all old orgs * test: add missing fixtures * feat: support multiple line items for CC charge exports (#677) * feat: support multiple line items for CC charge exports * test: update tests to account for the new function signature * test: update tests and fixtures Account for tax balancing and credit card charge lineitems changes * refactor: move url out of `for` loop (cherry picked from commit c0309e8)
Clickup
https://app.clickup.com/t/86cx0vh5n
Summary by CodeRabbit
New Features
bank_transaction_id, to enhance expense tracking.split_expense_grouping, to specify how expenses should be grouped when split.bank_transaction_id.Bug Fixes