feat: make db changes and fixture updates to support split expenses #671
Conversation
WalkthroughThis pull request introduces new fields to the Changes
Possibly related PRs
Suggested labels
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 (
|
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: 0
🧹 Outside diff range and nitpick comments (4)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
11637-11645: Update test data for bank_transaction_idAll expense records have
bank_transaction_id = NULL. If this field is meant to track bank transactions:
- Consider adding test cases with non-NULL values
- Document the scenarios where this field should/shouldn't be populated
- Ensure test coverage for both cases
tests/test_fyle/fixtures.py (1)
249-249: Consider using unique bank_transaction_ids for different expensesCurrently, all expenses use the same
bank_transaction_id: 'btxnqmATEEBkJ4'. This might not adequately test scenarios where unique transaction IDs are important. Consider using distinct IDs to better represent real-world data and catch potential issues with transaction tracking or grouping.- 'bank_transaction_id': 'btxnqmATEEBkJ4', + 'bank_transaction_id': 'btxnqmATEEBkJ4_1234',And similar unique suffixes for other expenses.
Also applies to: 290-290, 331-331, 859-859, 901-901
apps/fyle/models.py (2)
213-215: Add docstring for consistency.Consider adding a docstring to maintain consistency with other default value functions in the file.
def get_default_split_expense_grouping(): + """ + Returns the default value for split expense grouping + :return: str + """ return 'MULTIPLE_LINE_ITEM'
241-245: Fix indentation for consistency.The indentation of the field definition is inconsistent with other fields in the model.
- split_expense_grouping = models.CharField( - max_length=100, - default=get_default_split_expense_grouping, - choices=SPLIT_EXPENSE_GROUPING, help_text='specify line items for split expenses grouping' - ) + split_expense_grouping = models.CharField( + max_length=100, + default=get_default_split_expense_grouping, + choices=SPLIT_EXPENSE_GROUPING, + help_text='specify line items for split expenses grouping' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/fyle/migrations/0035_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/fixtures.py(7 hunks)tests/test_workspaces/test_apis/test_export_settings/fixtures.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/fyle/migrations/0035_support_split_expense_grouping.py
🔇 Additional comments (11)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
962-963:
Review NOT NULL constraint implementation
The new split_expense_grouping column is marked as NOT NULL but:
- There's no DEFAULT value specified
- No CHECK constraint to validate allowed values
- Existing records would need a value during migration
Consider:
- Adding a DEFAULT value
- Adding a CHECK constraint to validate values
- Documenting the migration strategy for existing records
#!/bin/bash
# Check if migration handles existing records
fd -t f -e py . -E "*.pyc" | xargs rg -A 10 "split_expense_grouping"
# Look for enum/choices definition
fd -t f -e py . -E "*.pyc" | xargs rg "class.*ExpenseGroupSettings|SPLIT_EXPENSE_GROUPING"1153-1154: Consider indexing bank_transaction_id
The bank_transaction_id column might benefit from an index if it's used for lookups or as a foreign key. Also, consider:
- Adding a unique constraint if this ID should be unique
- Adding a foreign key constraint if it references another table
- Documenting the nullability requirements as all sample data shows NULL values
#!/bin/bash
# Check if bank_transaction_id is used in queries
fd -t f -e py . -E "*.pyc" | xargs rg "bank_transaction_id"
# Look for related models/tables
fd -t f -e sql . | xargs rg "bank.*transaction"11582-11585: Verify data migration strategy
All expense_group_settings records use 'MULTIPLE_LINE_ITEM' for split_expense_grouping. Consider:
- Documenting if this is the intended default value
- Verifying if other values are supported
- Updating test data to cover different scenarios if multiple values are supported
#!/bin/bash
# Look for other possible values in code
fd -t f -e py . -E "*.pyc" | xargs rg "MULTIPLE_LINE_ITEM|split_expense_grouping.*=|split_expense_grouping.*choices"5-5: Verify PostgreSQL version downgrade compatibility
The PostgreSQL version has been downgraded from 15.9 to 15.7. While both versions are from the same major release (15.x), ensure this matches your development and production environments to avoid potential compatibility issues.
✅ Verification successful
PostgreSQL version change appears safe to proceed
The version change from 15.9 to 15.7 is a minor version downgrade within the same major version (15.x). After analyzing the codebase:
- No specific PostgreSQL version requirements are documented
- No version-specific PostgreSQL features are being used
- Both versions are recent and part of the same major release, maintaining backward compatibility
- The change is only in test fixtures, not affecting production environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if README or documentation specifies required PostgreSQL version
fd -t f -e md . | xargs rg -i "postgres(ql)?\s*(version|requirement)"
# Check if there are any version-specific PostgreSQL features in use
rg -i '\-\-\s*Requires PostgreSQL'
Length of output: 5094
Script:
#!/bin/bash
# Check for PostgreSQL version in Docker files or configuration
fd -t f "Dockerfile|docker-compose.*\.yml|\.env.*" | xargs rg -i "postgres(ql)?\s*:\s*\d+\.\d+"
# Check for any database configuration files
fd -t f -e py . | xargs rg -i "postgres(ql)?\s*:\s*\d+\.\d+"
# Check for any version-specific PostgreSQL features in SQL files
fd -t f -e sql . | xargs rg -i "requires\s+postgres(ql)?\s*>\s*\d+\.\d+"
Length of output: 235
tests/test_fyle/fixtures.py (2)
1000-1001: Verify all possible values for split_expense_grouping
The addition of "split_expense_grouping": "MULTIPLE_LINE_ITEM" suggests this is a choice field. Let's verify all possible values to ensure test coverage.
#!/bin/bash
# Search for the model definition and any choices/enums for split_expense_grouping
ast-grep --pattern 'class ExpenseGroupSettings:
$$$
split_expense_grouping = $$$
$$$'
# Also check for any migrations defining choices
rg -l "split_expense_grouping.*choices" --type py1105-1106: Verify expense state configurations for all expense types
The changes add both ccc_expense_state and split_expense_grouping configurations. Let's ensure all expense types and states are properly configured.
tests/test_workspaces/test_apis/test_export_settings/fixtures.py (2)
78-79: LGTM! Response matches request payload.
The split_expense_grouping field in the response correctly mirrors the configuration from the request payload, maintaining consistency.
29-30: Consider adding test cases for different split expense grouping values.
While the current test case covers the 'MULTIPLE_LINE_ITEM' scenario, it would be beneficial to add test cases that validate the behavior with different possible values of split_expense_grouping.
apps/fyle/models.py (3)
75-76: LGTM! Well-structured choices constant.
The SPLIT_EXPENSE_GROUPING constant follows Django's choice field convention and provides clear, self-documenting options.
108-108: LGTM! Well-defined field for bank transaction tracking.
The bank_transaction_id field follows Django model field best practices with appropriate nullability and length constraints.
325-326: Verify split_expense_grouping value validation.
The update_expense_group_settings method accepts the new field but doesn't explicitly validate that the value matches the choices. While Django's model field will handle this, consider adding explicit validation for better error handling.
JustARatherRidiculouslyLongUsername
force-pushed
the
ut-split-expense-grouping
branch
from
2456e40 to
a54d312
Compare
JustARatherRidiculouslyLongUsername
force-pushed
the
split-expenses-db-changes
branch
from
a41a418 to
b24e6c6
Compare
Clickup
https://app.clickup.com/t/86cx0vh5m
Summary by CodeRabbit
New Features
bank_transaction_idfor tracking bank transactions in expenses.split_expense_groupingto enhance expense grouping options.Bug Fixes
Tests