test: add unit tests for split expense grouping and update existing fixtures #401
Conversation
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces new fields to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (8)
tests/test_fyle/fixtures.py (1)
286-462: Well-structured test data for split expensesThe new
ccc_split_expensestest data effectively models a real-world split transaction scenario:
- Same
bank_transaction_idlinking related expenses- Amounts distributed across different projects
- Consistent transaction metadata (vendor, date, card)
Consider adding a comment explaining the test scenario this data represents.
+ # Test data representing a $103.234 credit card transaction at Dominos Pizza + # split across Project 8 ($21.208) and Project 10 ($82.026 split into 3 expenses) "ccc_split_expenses": [tests/test_fyle/conftest.py (1)
38-52: Consider improving the fixture's maintainability.The fixture uses hardcoded strings for configuration values. Consider:
- Using constants for the configuration values.
- Adding docstring to explain the fixture's purpose.
- Adding type hints for better code maintainability.
Here's a suggested improvement:
from typing import Callable, Any @pytest.fixture def update_config_for_split_expense_grouping(db) -> Callable[[Any, ExpenseGroupSettings], None]: """Updates configuration for split expense grouping test scenarios. Args: general_settings: General settings object to update expense_group_settings: ExpenseGroupSettings object to update """ def _update_config_for_split_expense_grouping( general_settings: Any, expense_group_settings: ExpenseGroupSettings ) -> None: general_settings.corporate_credit_card_expenses_object = 'BANK TRANSACTION' general_settings.save() 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() return _update_config_for_split_expense_groupingtests/test_fyle/test_models.py (4)
110-140: Consider enhancing test coverage with edge cases.The test effectively validates the basic functionality of expense grouping without bank transaction IDs. However, consider adding:
- Edge cases (e.g., empty expense list)
- Verification of expense group properties beyond count (e.g., group fields, timestamps)
143-175: Consider verifying group composition.The test effectively validates the number of groups created for different grouping types. To make it more robust, consider:
- Verifying which expenses are grouped together
- Asserting the properties of each group to ensure correct grouping logic
Example assertion:
for group in groups: expenses_in_group = group.expenses.all() if len(expenses_in_group) > 1: # All expenses in the group should have the same bank_transaction_id assert len(set(e.bank_transaction_id for e in expenses_in_group)) == 1
178-210: Consider additional test scenarios and group verification.The test effectively validates the grouping of paired expenses. To enhance coverage, consider:
- Testing uneven pairs (e.g., 3 expenses with same ID and 1 with different)
- Verifying the composition of each group to ensure correct pairing
110-210: Consider adding tests for the update_config_for_split_expense_grouping fixture.While the tests effectively cover the split expense grouping functionality, consider adding tests to verify the behavior of the
update_config_for_split_expense_groupingfixture itself. This would ensure that the configuration setup remains reliable.apps/fyle/models.py (2)
131-131: Consider adding an index for bank_transaction_id field.While the field is well-defined, adding a database index would optimize queries that filter or join on this field, especially if it's frequently used in lookups.
- bank_transaction_id = models.CharField(max_length=255, null=True, blank=True, help_text='Bank Transaction ID') + bank_transaction_id = models.CharField(max_length=255, null=True, blank=True, help_text='Bank Transaction ID', db_index=True)Also applies to: 204-204
266-268: Document the default value choice.Please add a docstring explaining why 'MULTIPLE_LINE_ITEM' was chosen as the default value. This helps future maintainers understand the reasoning behind this decision.
def get_default_split_expense_grouping(): + """ + Returns the default split expense grouping option. + Default is 'MULTIPLE_LINE_ITEM' because [add reasoning here]. + """ return 'MULTIPLE_LINE_ITEM'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
apps/fyle/migrations/0021_support_split_expense_grouping.py(1 hunks)apps/fyle/models.py(6 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(7 hunks)tests/test_fyle/conftest.py(1 hunks)tests/test_fyle/fixtures.py(6 hunks)tests/test_fyle/test_models.py(2 hunks)tests/test_workspaces/fixtures.py(1 hunks)tests/test_workspaces/test_apis/test_clone_settings/fixtures.py(2 hunks)tests/test_workspaces/test_apis/test_export_settings/fixtures.py(2 hunks)
🔥 Files not summarized due to errors (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (22)
tests/test_fyle/fixtures.py (3)
239-240: LGTM: Consistent addition of bank_transaction_id field
The bank_transaction_id field has been consistently added across all expense dictionaries with appropriate null initialization, matching the nullable CharField addition in the expense model.
Also applies to: 505-505, 782-782
286-462: Verify test coverage for split expense scenarios
The fixture provides comprehensive test data for split expenses. Let's verify that corresponding test cases exist utilizing this data.
#!/bin/bash
# Search for test cases using the fixture data
rg -l "ccc_split_expenses|split_expense_grouping" tests/Also applies to: 552-552, 583-583
552-552: Verify split_expense_grouping enum values
The split_expense_grouping field is consistently set to "MULTIPLE_LINE_ITEM". Let's verify this matches the allowed choices in the model.
Also applies to: 583-583
✅ Verification successful
split_expense_grouping value matches allowed choices
The value "MULTIPLE_LINE_ITEM" is a valid choice as defined in the model's SPLIT_EXPENSE_GROUPING tuple: (('SINGLE_LINE_ITEM', 'SINGLE_LINE_ITEM'), ('MULTIPLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM')).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the model definition to verify enum choices
ast-grep --pattern 'split_expense_grouping = models.CharField(choices=$_)'
Length of output: 492
Script:
#!/bin/bash
# Search for split_expense_grouping model field definition
rg -A 5 "split_expense_grouping.*CharField"
Length of output: 579
Script:
#!/bin/bash
# Find the SPLIT_EXPENSE_GROUPING choices definition
rg -B 5 "SPLIT_EXPENSE_GROUPING.*=" apps/fyle/
Length of output: 973
apps/fyle/migrations/0021_support_split_expense_grouping.py (3)
1-11: LGTM: Migration setup is properly configured.
The migration file follows Django conventions with proper imports and dependency specification.
19-23: Verify the default value implementation and consider improvements.
A few points to consider:
- The default value is set via
get_default_split_expense_grouping, but we should verify its implementation - Consider using Django's TextChoices for better maintainability of the choices
- Existing records will use the default value, verify if this is the intended behavior
Let's verify the default value implementation:
#!/bin/bash
# Check the implementation of get_default_split_expense_grouping
ast-grep --pattern 'def get_default_split_expense_grouping():
$$$
'
# Check if there are any existing usages of these values
rg -A 2 "SINGLE_LINE_ITEM|MULTIPLE_LINE_ITEM" --type pyConsider refactoring to use Django's TextChoices for better maintainability:
class SplitExpenseGrouping(models.TextChoices):
SINGLE_LINE_ITEM = 'SINGLE_LINE_ITEM', 'Single Line Item'
MULTIPLE_LINE_ITEM = 'MULTIPLE_LINE_ITEM', 'Multiple Line Item'14-18: LGTM: bank_transaction_id field is properly defined.
The field specifications are appropriate for storing bank transaction IDs. The field is correctly made optional which is good for backward compatibility.
Let's verify if there are any existing references to this field that might need updates:
✅ Verification successful
LGTM: bank_transaction_id field is properly integrated in the codebase
The field is already being used appropriately across the codebase:
- Properly defined in the Expense model with consistent specifications
- Used in test fixtures with both null and non-null values
- Has corresponding test coverage for split expense grouping scenarios
- Referenced correctly in expense object creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing references to bank_transaction_id
# to ensure we haven't missed any required updates
rg "bank_transaction_id" --type py
Length of output: 4700
Script:
#!/bin/bash
# Let's search for any references to bank_transaction_id in models, serializers, and views
rg "bank_transaction_id" --type py -A 2
Length of output: 4093
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (2)
49-49: LGTM! Consistent with request structure.
The addition maintains symmetry between request and response structures, which is essential for testing both scenarios.
10-10: LGTM! Verify the choice value.
The addition of split_expense_grouping with value "MULTIPLE_LINE_ITEM" is correctly placed in the expense group settings.
Let's verify that this is a valid choice in the model:
✅ Verification successful
The choice value is valid and correctly used
The value "MULTIPLE_LINE_ITEM" is a valid choice for split_expense_grouping as defined in apps/fyle/models.py. The model explicitly defines the choices as:
SPLIT_EXPENSE_GROUPING = (('SINGLE_LINE_ITEM', 'SINGLE_LINE_ITEM'), ('MULTIPLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM'))It's also set as the default value via get_default_split_expense_grouping function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that MULTIPLE_LINE_ITEM is a valid choice in the expensegroupsettings model
ast-grep --pattern 'class ExpenseGroupSettings:
$$$
split_expense_grouping = $$$choices$$$
$$$
'
Length of output: 105
Script:
#!/bin/bash
# Let's try a broader search for the model definition and choices
rg -A 10 "class ExpenseGroupSettings"
Length of output: 2724
Script:
#!/bin/bash
# Search for any enum or choices related to split_expense_grouping
rg -i "split_expense.*grouping.*choices|MULTIPLE_LINE_ITEM"
Length of output: 2373
Script:
#!/bin/bash
# Look for model files that might contain this definition
fd "models|choices|enums" --type f
Length of output: 301
tests/test_workspaces/fixtures.py (2)
99-99: LGTM: Bank transaction ID field added correctly.
The addition of bank_transaction_id with a None default value is appropriate for the expense fixture.
Line range hint 1-142: Add test coverage for split expense grouping.
The fixture is missing the split_expense_grouping field that was mentioned in the AI summary. Since this PR focuses on adding unit tests for split expense grouping, we should ensure the fixtures include this new field.
Let's verify if this field exists in other test files:
Would you like me to help add the necessary test fixtures for split expense grouping?
tests/test_workspaces/test_apis/test_clone_settings/fixtures.py (2)
99-99: LGTM! Maintains consistency with response structure.
The split_expense_grouping configuration is correctly mirrored in the base clone_settings, maintaining symmetry with the response structure.
30-30: LGTM! Verify enum value validity.
The addition of split_expense_grouping configuration in clone_settings_response is well-placed within the expense_group_settings section.
✅ Verification successful
✓ Valid enum value confirmed
"MULTIPLE_LINE_ITEM" is a valid choice in the SPLIT_EXPENSE_GROUPING enum defined in the model, alongside "SINGLE_LINE_ITEM". The test fixture value is correctly using one of the allowed options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that "MULTIPLE_LINE_ITEM" is a valid enum value in the model
ast-grep --pattern 'class $_(models.Model):
$$$
split_expense_grouping = $$$
$$$'
Length of output: 10817
Script:
#!/bin/bash
# Find the SPLIT_EXPENSE_GROUPING choices definition
rg "SPLIT_EXPENSE_GROUPING.*=" -A 5
Length of output: 878
tests/test_fyle/test_models.py (1)
10-10: LGTM!
The import of WorkspaceGeneralSettings is necessary for the new test functions and follows the project's import style.
apps/fyle/models.py (2)
65-66: LGTM! Well-defined choices for split expense grouping.
The constant follows Django's choice field convention and uses clear, self-documenting values.
324-324: Consider adding validation for split expense grouping changes.
The split_expense_grouping field is well-defined, but consider adding validation rules to ensure that changing between 'SINGLE_LINE_ITEM' and 'MULTIPLE_LINE_ITEM' doesn't cause issues with existing expense groups.
Also applies to: 432-432
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (7)
2641-2641: LGTM! Migration sequence is properly maintained.
The migration 0021_support_split_expense_grouping is correctly sequenced after previous migrations.
4882-4892: LGTM! Test data is comprehensive and well-structured.
The test data covers various scenarios with consistent default values for the new fields.
5179-5179: LGTM! Sequence value is correctly updated.
The django_migrations sequence is properly set to 156, matching the latest migration number.
6796-6796: LGTM! Standard file ending.
4837-4838: Consider documenting allowed values for split_expense_grouping.
The test data shows MULTIPLE_LINE_ITEM as a value for split_expense_grouping. Consider documenting all possible values for this field in the model or constants file.
#!/bin/bash
# Description: Look for documentation or enum definition for split_expense_grouping values
# Test: Search for files that might contain the field's allowed values
rg -l "MULTIPLE_LINE_ITEM" --type py818-819: Verify migration strategy for NOT NULL constraint.
The split_expense_grouping column is added with a NOT NULL constraint. Ensure that the corresponding migration handles existing records by providing a default value, or verify that this is a fresh test database setup.
✅ Verification successful
Migration strategy for NOT NULL constraint is properly handled
The migration file shows that split_expense_grouping is added with a default value provided by get_default_split_expense_grouping function and has predefined choices ('SINGLE_LINE_ITEM', 'MULTIPLE_LINE_ITEM'). The test database dump also shows existing data with 'MULTIPLE_LINE_ITEM' value. This confirms that the NOT NULL constraint is safely handled in the migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's a corresponding migration file that handles the NOT NULL constraint
# Test: Look for migration files related to split_expense_grouping
rg -l "split_expense_grouping" --type py
Length of output: 388
Script:
#!/bin/bash
# Let's check the migration file content to verify the migration strategy
cat apps/fyle/migrations/0021_support_split_expense_grouping.py
# Also check the test fixture file to understand the context
cat tests/sql_fixtures/reset_db_fixtures/reset_db.sql | grep -A 10 -B 10 "split_expense_grouping"
Length of output: 4172
947-948: Verify migration strategy for NOT NULL constraint.
The paid_on_fyle column is added with a NOT NULL constraint. Ensure that the corresponding migration handles existing records by providing a default value, or verify that this is a fresh test database setup.
✅ Verification successful
Migration strategy for NOT NULL constraint is correct
The concern is resolved as the migration file 0019_expense_paid_on_fyle.py properly handles the NOT NULL constraint by providing a default value of False. Additionally, this file is in the test fixtures directory which is used for setting up test databases, so existing production data is not affected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's a corresponding migration file that handles the NOT NULL constraint
# Test: Look for migration files related to paid_on_fyle
rg -l "paid_on_fyle" --type py
Length of output: 388
Script:
#!/bin/bash
# Let's check the content of the reset_db.sql file to understand if this is a test database setup
rg -A 10 -B 10 "paid_on_fyle" tests/sql_fixtures/reset_db_fixtures/reset_db.sql
# Also search for any migration files in the apps directory
fd migration apps/
Length of output: 8394
Script:
#!/bin/bash
# Let's check the content of the migration file that added paid_on_fyle column
rg -A 10 "paid_on_fyle" apps/fyle/migrations/0019_expense_paid_on_fyle.py
Length of output: 221
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
ruuushhh
requested review from
ashwin1111
and removed request for
ashwin1111
JustARatherRidiculouslyLongUsername
changed the base branch from
master
to
split-expense-grouping
…alizer (#404) * feat: add `split_expense_grouping` to the expense group settings serializer * feat: implement split expense grouping functionality (#405) * feat: implement split expense grouping functionality * fix: skip only `expense_id` and `expense_number` while grouping while defining the fields to group split expenses by, skip only `expense_id` and `expense_number`, NOT `spent_at` or `posted_at` * feat: add script for split expense grouping (#406) Set the default split expense grouping config to `SINGLE_LINE_ITEM` for all old orgs
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
JustARatherRidiculouslyLongUsername
merged commit 0b487e5
into
split-expense-grouping
* feat: add fields to support split expense grouping * fix: update sql fixture * test: add unit tests for split expense grouping and update existing fixtures (#401) * test: add unit tests for split expense grouping and update existing fixtures * refactor: lint * feat: add `split_expense_grouping` to the expense group settings serializer (#404) * feat: add `split_expense_grouping` to the expense group settings serializer * feat: implement split expense grouping functionality (#405) * feat: implement split expense grouping functionality * fix: skip only `expense_id` and `expense_number` while grouping while defining the fields to group split expenses by, skip only `expense_id` and `expense_number`, NOT `spent_at` or `posted_at` * feat: add script for split expense grouping (#406) Set the default split expense grouping config to `SINGLE_LINE_ITEM` for all old orgs
* feat: add fields to support split expense grouping * fix: update sql fixture * test: add unit tests for split expense grouping and update existing fixtures (#401) * test: add unit tests for split expense grouping and update existing fixtures * refactor: lint * feat: add `split_expense_grouping` to the expense group settings serializer (#404) * feat: add `split_expense_grouping` to the expense group settings serializer * feat: implement split expense grouping functionality (#405) * feat: implement split expense grouping functionality * fix: skip only `expense_id` and `expense_number` while grouping while defining the fields to group split expenses by, skip only `expense_id` and `expense_number`, NOT `spent_at` or `posted_at` * feat: add script for split expense grouping (#406) Set the default split expense grouping config to `SINGLE_LINE_ITEM` for all old orgs (cherry picked from commit 86effd4)
Clickup
https://app.clickup.com/t/86cx07z0e