feat: handle posted at #661
Conversation
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes introduce a new Boolean field named Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@anishfyle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 58 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: 2
🧹 Outside diff range and nitpick comments (1)
apps/fyle/models.py (1)
110-110: Consider enhancing the field definition for better performance and clarity.The field placement and type are appropriate, but consider these improvements:
- Add db_index for better query performance since this field will likely be used in filters
- Enhance the help text to be more descriptive about when and why this flag is set
- is_posted_at_null = models.BooleanField(default=False, help_text='Flag check if posted at is null or not') + is_posted_at_null = models.BooleanField( + default=False, + db_index=True, + help_text='Flag indicating if posted_at is null and spent_at should be used instead' + )
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
12727-12734: Clarify inclusion of task_logs constraintsThe addition of unique and foreign key constraints to
task_logstable appears unrelated to the PR's objective of handling posted_at. While these are good database practices, please clarify:
- Why these constraints are included in this PR
- Impact assessment on existing data
- Whether this should be in a separate PR
Also applies to: 13773-13780
tests/test_netsuite/conftest.py (2)
65-67: Consider improving test isolation approach.While clearing TaskLogs helps with test isolation, consider these improvements:
- Use the expense_group's id instead of hardcoding
expense_group_id=1:- TaskLog.objects.filter(expense_group_id=1).delete() + TaskLog.objects.filter(expense_group_id=expense_group.id).delete()
- Add a more descriptive comment explaining why TaskLogs need to be cleared:
- # Clear existing TaskLogs for this expense group + # Clear existing TaskLogs to prevent interference from previous test runs and ensure clean state
65-67: Add test coverage for the newis_posted_at_nullfield.The PR introduces a new
is_posted_at_nullfield to handle nullposted_atvalues, but the test fixtures don't cover this functionality. Consider adding test cases to verify:
- Setting
is_posted_at_null=Truewhenposted_atis null- Using
spent_atas fallback whenposted_atis null- Setting
is_posted_at_null=Falsewhenposted_athas a valueWould you like me to help generate the test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/fyle/migrations/0034_expense_is_posted_at_null.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(7 hunks)tests/test_netsuite/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/fyle/migrations/0034_expense_is_posted_at_null.py
🔇 Additional comments (4)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
5-6: LGTM: PostgreSQL version update
Routine maintenance update from 15.7 to 15.9.
7990-7991: Verify scope of tasks migration
While fyle.0034_expense_is_posted_at_null aligns with PR objective, the inclusion of tasks.0012_alter_tasklog_expense_group seems unrelated to posted_at handling. Please clarify if this is intentional.
11633-11641: Verify data consistency for posted_at handling
The sample data shows all records have is_posted_at_null set to false, but some records have posted_at as NULL. This might need reconciliation to ensure the flag accurately reflects the data state.
✅ Verification successful
Based on the output, I can now generate the final response since I have enough information to verify the concern.
No inconsistency found between posted_at and is_posted_at_null values
The data shows consistent handling where:
- All records have
is_posted_at_nullset tofalse - All records have
posted_atexplicitly set to\N(NULL) - This is the expected behavior as per the schema design where
is_posted_at_nullindicates if the record should be considered for processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistencies between posted_at and is_posted_at_null values
rg "COPY.*expenses.*posted_at.*is_posted_at_null" -A 10
Length of output: 5093
1151-1152: Verify default value handling for is_posted_at_null
The new column is correctly added with NOT NULL constraint, but we should verify how existing records are handled.
* feat: auto enable accounting period * reset db * platform connector ver
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/workspaces/migrations/0040_alter_configuration_change_accounting_period.py(1 hunks)apps/workspaces/models.py(1 hunks)requirements.txt(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/workspaces/migrations/0040_alter_configuration_change_accounting_period.py
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql
* handle posted at * migrations, reset db, conftest * feat: auto enable accounting period (#668) * feat: auto enable accounting period * reset db * platform connector ver
Description
Clickup
https://app.clickup.com/t/86cx1az88
Summary by CodeRabbit
is_posted_at_null, in the Expense model to indicate if theposted_atfield is null, enhancing tracking capabilities.is_posted_at_nullcolumn in the expenses table.change_accounting_periodfield in the Configuration model toTrue, indicating that the accounting period will now change by default.