Max retry payment Limit with test #548
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes involve enhancements to the Changes
Possibly related PRs
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 (
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #548 +/- ##
==========================================
+ Coverage 93.00% 93.44% +0.43%
==========================================
Files 62 62
Lines 5535 5552 +17
==========================================
+ Hits 5148 5188 +40
+ Misses 387 364 -23
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/sage_intacct/tasks.py (3 hunks)
- tests/test_sageintacct/test_tasks.py (2 hunks)
Additional context used
Ruff
tests/test_sageintacct/test_tasks.py
2-2:
dateutil.relativedelta.relativedeltaimported but unusedRemove unused import:
dateutil.relativedelta.relativedelta(F401)
1475-1475:
create_billmay be undefined, or defined from star imports(F405)
1477-1477:
Billmay be undefined, or defined from star imports(F405)
1487-1487:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1489-1489:
timezonemay be undefined, or defined from star imports(F405)
1492-1492:
timedeltamay be undefined, or defined from star imports(F405)
1493-1493:
timedeltamay be undefined, or defined from star imports(F405)
1498-1498:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1504-1504:
timedeltamay be undefined, or defined from star imports(F405)
1505-1505:
timedeltamay be undefined, or defined from star imports(F405)
1507-1507:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1513-1513:
timedeltamay be undefined, or defined from star imports(F405)
1514-1514:
timedeltamay be undefined, or defined from star imports(F405)
1516-1516:
create_ap_paymentmay be undefined, or defined from star imports(F405)
Additional comments not posted (2)
tests/test_sageintacct/test_tasks.py (1)
1430-1518: Review of the new test functiontest_skipping_create_ap_payment.
Mocking and Patching:
- The function extensively uses the
mockerlibrary to patch various API calls, ensuring that the test does not make actual calls to external services. This is a good practice as it isolates the test environment from external dependencies.Test Logic:
- The test sets up the necessary mocks and then manipulates the
TaskLogstatus andReimbursementstates to simulate different scenarios. This approach is effective for testing the behavior of thecreate_ap_paymentfunction under controlled conditions.Time Manipulation:
- The test manipulates the
created_atandupdated_attimestamps of theTaskLogto test the function's response to varying time conditions. This is crucial for ensuring that the function behaves correctly over time, especially for edge cases.Assertions:
- The test checks that the status of the
TaskLogis set to 'FAILED' under all tested conditions. This is a clear and direct way to verify the expected outcome of the test scenarios.Code Quality:
- The test function is well-structured and follows good coding practices. The use of descriptive variable names and clear comments enhances readability and maintainability.
Potential Improvements:
- Consider adding more comments explaining why each specific condition is tested, especially for those unfamiliar with the business logic.
- It might be beneficial to test additional edge cases or error conditions that could occur in production.
Overall, the test function is well-implemented and serves its purpose effectively. It enhances the test coverage for the payment creation functionality, ensuring that edge cases related to task timing are adequately addressed.
The code changes are approved.
Tools
Ruff
1475-1475:
create_billmay be undefined, or defined from star imports(F405)
1477-1477:
Billmay be undefined, or defined from star imports(F405)
1487-1487:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1489-1489:
timezonemay be undefined, or defined from star imports(F405)
1492-1492:
timedeltamay be undefined, or defined from star imports(F405)
1493-1493:
timedeltamay be undefined, or defined from star imports(F405)
1498-1498:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1504-1504:
timedeltamay be undefined, or defined from star imports(F405)
1505-1505:
timedeltamay be undefined, or defined from star imports(F405)
1507-1507:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1513-1513:
timedeltamay be undefined, or defined from star imports(F405)
1514-1514:
timedeltamay be undefined, or defined from star imports(F405)
1516-1516:
create_ap_paymentmay be undefined, or defined from star imports(F405)
apps/sage_intacct/tasks.py (1)
1096-1128: Review: Enhanced logic for managing task logs increate_ap_payment.The modifications to
create_ap_paymentintroduce a more nuanced control flow based on the age of thetask_log. The use ofrelativedeltafor date calculations is appropriate and improves the flexibility of time comparisons. However, the logic is somewhat complex and could benefit from further simplification or documentation to improve maintainability.
- Correctness: The logic appears correct based on the provided context, but ensure all edge cases are handled, especially around date boundaries.
- Performance: The function iterates over bills and performs database operations within a loop, which could be optimized.
- Maintainability: The nested conditions make the function somewhat difficult to maintain. Consider refactoring to reduce complexity or adding more comments to explain the logic.
Consider simplifying the logic by breaking down the function into smaller, more manageable parts or by using helper functions to handle the date comparisons and task log updates.
Run the following script to verify the function usage:
Verification successful
Verification Successful:
create_ap_paymentFunction Usage and TestingThe
create_ap_paymentfunction is well-integrated into the codebase and is covered by tests in multiple files. This ensures that the changes to the function are verified and that it behaves as expected across different scenarios.
- Test Coverage: The function is tested in
tests/test_sageintacct/test_tasks.py,tests/test_sageintacct/test_utils.py, andtests/test_sageintacct/test_models.py.- Application Usage: It is used in
apps/sage_intacct/tasks.py,apps/sage_intacct/views.py, andapps/sage_intacct/queue.py.These findings confirm that the function's logic is correctly implemented and tested. No further action is required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `create_ap_payment` match the new logic. # Test: Search for the function usage. Expect: Only occurrences of the new logic. rg --type python -A 5 $'create_ap_payment'Length of output: 72
Script:
#!/bin/bash # Description: Verify all function calls to `create_ap_payment` match the new logic. # Test: Search for the function usage. Expect: Only occurrences of the new logic. rg -t py -A 5 'create_ap_payment'Length of output: 15688
apps/sage_intacct/tasks.py
Outdated
| @@ -1091,11 +1093,39 @@ def create_ap_payment(workspace_id): | |||
| expense_group__fund_source='PERSONAL' | |||
| ).all() | |||
|
|
|||
| print('bills length', len(bills)) | |||
apps/sage_intacct/tasks.py
Outdated
| @@ -1105,6 +1135,7 @@ def create_ap_payment(workspace_id): | |||
| } | |||
| ) | |||
|
|
|||
|
|
|||
apps/sage_intacct/tasks.py
Outdated
|
|
||
| task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(bill.expense_group.id)).first() | ||
|
|
||
| if task_log: | ||
| now = timezone.now() | ||
|
|
||
| if now - relativedelta(months=2) > task_log.created_at: | ||
| task_log.status = 'FAILED' | ||
| task_log.save() | ||
| continue | ||
|
|
||
| elif now - relativedelta(months=1) > task_log.created_at and now - relativedelta(months=2) < task_log.created_at: | ||
| # if updated_at is within 1 months will be skipped | ||
| if task_log.updated_at > now - relativedelta(months=1): | ||
| task_log.status = 'FAILED' | ||
| task_log.save() | ||
| continue | ||
|
|
||
| # If created is within 1 month | ||
| elif now - relativedelta(months=1) < task_log.created_at: | ||
| # Skip if updated within the last week | ||
| if task_log.updated_at > now - relativedelta(weeks=1): | ||
| task_log.status = 'FAILED' | ||
| task_log.save() | ||
| continue |
There was a problem hiding this comment.
move this whole thing to function
apps/sage_intacct/tasks.py
Outdated
| if bills: | ||
| for bill in bills: | ||
| expense_group_reimbursement_status = check_expenses_reimbursement_status( | ||
| bill.expense_group.expenses.all(), workspace_id=workspace_id, platform=platform, filter_credit_expenses=filter_credit_expenses) | ||
| if expense_group_reimbursement_status: | ||
|
|
||
| task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(bill.expense_group.id)).first() |
There was a problem hiding this comment.
add workspace id, type filter as well
apps/sage_intacct/tasks.py
Outdated
|
|
||
| if now - relativedelta(months=2) > task_log.created_at: | ||
| task_log.status = 'FAILED' | ||
| task_log.save() |
There was a problem hiding this comment.
let's also have 1 new column in bill and expense_reports table, is_retired and set it to true. By default it can be false and we can add this to the query when it runs checks - give me bills where is_paid_in_fyle true and paid_in_intacct false and is_retired is false. From next run we end up skipping even these checks and have a way in DB for us to know if customer complaints.
apps/sage_intacct/tasks.py
Outdated
| now = timezone.now() | ||
|
|
||
| if now - relativedelta(months=2) > task_log.created_at: | ||
| task_log.status = 'FAILED' |
There was a problem hiding this comment.
status would be failed already, no need to update anything here
apps/sage_intacct/tasks.py
Outdated
| elif now - relativedelta(months=1) > task_log.created_at and now - relativedelta(months=2) < task_log.created_at: | ||
| # if updated_at is within 1 months will be skipped | ||
| if task_log.updated_at > now - relativedelta(months=1): | ||
| task_log.status = 'FAILED' |
There was a problem hiding this comment.
status would be failed already, no need to update anything here
apps/sage_intacct/tasks.py
Outdated
| elif now - relativedelta(months=1) < task_log.created_at: | ||
| # Skip if updated within the last week | ||
| if task_log.updated_at > now - relativedelta(weeks=1): | ||
| task_log.status = 'FAILED' |
There was a problem hiding this comment.
status would be failed already, no need to update anything here
tests/test_sageintacct/test_tasks.py
Outdated
| ) | ||
| create_ap_payment(workspace_id) | ||
| task_log.refresh_from_db() | ||
| assert task_log.status == 'FAILED' |
There was a problem hiding this comment.
not right metric to assert, we should check updated_at - if it's not updated then it means we skipped
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
tests/test_sageintacct/test_tasks.py (1)
1430-1522: Test function looks good!The
test_skipping_create_ap_paymentfunction is well-structured and covers different scenarios for skipping thecreate_ap_paymentfunction based on the age of theTaskLog. It sets up the necessary mocks, updates theTaskLogtimestamps to simulate the desired conditions, and makes appropriate assertions to verify the behavior.To address the static analysis hints:
- Remove the unused import
dateutil.relativedelta.relativedeltaon line 2.- Add the missing imports for
Bill,create_bill,create_ap_payment,timezone, andtimedeltaat the beginning of the file:from datetime import datetime, timedelta, timezone from apps.sage_intacct.models import Bill from apps.sage_intacct.tasks import create_bill, create_ap_paymentTools
Ruff
1475-1475:
create_billmay be undefined, or defined from star imports(F405)
1477-1477:
Billmay be undefined, or defined from star imports(F405)
1487-1487:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1489-1489:
timezonemay be undefined, or defined from star imports(F405)
1490-1490:
timedeltamay be undefined, or defined from star imports(F405)
1493-1493:
timedeltamay be undefined, or defined from star imports(F405)
1499-1499:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1504-1504:
timedeltamay be undefined, or defined from star imports(F405)
1507-1507:
timedeltamay be undefined, or defined from star imports(F405)
1510-1510:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1514-1514:
timedeltamay be undefined, or defined from star imports(F405)
1517-1517:
timedeltamay be undefined, or defined from star imports(F405)
1520-1520:
create_ap_paymentmay be undefined, or defined from star imports(F405)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/test_sageintacct/test_tasks.py (2 hunks)
Additional context used
Ruff
tests/test_sageintacct/test_tasks.py
2-2:
dateutil.relativedelta.relativedeltaimported but unusedRemove unused import:
dateutil.relativedelta.relativedelta(F401)
1475-1475:
create_billmay be undefined, or defined from star imports(F405)
1477-1477:
Billmay be undefined, or defined from star imports(F405)
1487-1487:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1489-1489:
timezonemay be undefined, or defined from star imports(F405)
1490-1490:
timedeltamay be undefined, or defined from star imports(F405)
1493-1493:
timedeltamay be undefined, or defined from star imports(F405)
1499-1499:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1504-1504:
timedeltamay be undefined, or defined from star imports(F405)
1507-1507:
timedeltamay be undefined, or defined from star imports(F405)
1510-1510:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1514-1514:
timedeltamay be undefined, or defined from star imports(F405)
1517-1517:
timedeltamay be undefined, or defined from star imports(F405)
1520-1520:
create_ap_paymentmay be undefined, or defined from star imports(F405)
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
apps/sage_intacct/tasks.py
Outdated
| @@ -1083,6 +1083,27 @@ def check_expenses_reimbursement_status(expenses, workspace_id, platform, filter | |||
| return is_paid | |||
|
|
|||
|
|
|||
| def validate_for_skipping_payment(expense_group_id, workspace_id): | |||
| task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(expense_group_id), workspace_id=workspace_id, type='CREATING_AP_PAYMENT').first() | |||
There was a problem hiding this comment.
type would be diff for ap payment and reimbursement
apps/sage_intacct/tasks.py
Outdated
| @@ -1093,38 +1114,17 @@ def create_ap_payment(workspace_id): | |||
| expense_group__fund_source='PERSONAL' | |||
| @@ -1250,6 +1272,13 @@ def create_sage_intacct_reimbursement(workspace_id): | |||
| expense_group_reimbursement_status = check_expenses_reimbursement_status( | |||
| expense_report.expense_group.expenses.all(), workspace_id=workspace_id, platform=platform, filter_credit_expenses=filter_credit_expenses) | |||
| if expense_group_reimbursement_status: | |||
|
|
|||
There was a problem hiding this comment.
same here, in 1268 - is_retired=False
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (2)
apps/sage_intacct/migrations/0028_auto_20240902_1511.py (2)
13-17: Fix the help text for clarity and grammar.The current help text is unclear and has a grammatical error. Update it to clearly convey the purpose of the field.
Apply this diff to fix the help text:
- field=models.BooleanField(default=False, help_text='Is Payment sync retried'), + field=models.BooleanField(default=False, help_text='Indicates if payment sync has been retired'),
18-22: Fix the help text for clarity and grammar.The current help text is unclear and has a grammatical error. Update it to clearly convey the purpose of the field.
Apply this diff to fix the help text:
- field=models.BooleanField(default=False, help_text='Is Payment sync retried'), + field=models.BooleanField(default=False, help_text='Indicates if payment sync has been retired'),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/sage_intacct/migrations/0028_auto_20240902_1511.py (1 hunks)
- apps/sage_intacct/models.py (2 hunks)
- apps/sage_intacct/tasks.py (6 hunks)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql (6 hunks)
- tests/test_sageintacct/test_tasks.py (6 hunks)
Additional context used
Ruff
tests/test_sageintacct/test_tasks.py
2-2:
dateutil.relativedelta.relativedeltaimported but unusedRemove unused import:
dateutil.relativedelta.relativedelta(F401)
458-458:
timezonemay be undefined, or defined from star imports(F405)
459-459:
timedeltamay be undefined, or defined from star imports(F405)
489-489:
create_sage_intacct_reimbursementmay be undefined, or defined from star imports(F405)
516-516:
timezonemay be undefined, or defined from star imports(F405)
517-517:
timedeltamay be undefined, or defined from star imports(F405)
1487-1487:
create_billmay be undefined, or defined from star imports(F405)
1489-1489:
Billmay be undefined, or defined from star imports(F405)
1499-1499:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1501-1501:
timezonemay be undefined, or defined from star imports(F405)
1502-1502:
timedeltamay be undefined, or defined from star imports(F405)
1505-1505:
timedeltamay be undefined, or defined from star imports(F405)
1511-1511:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1516-1516:
timedeltamay be undefined, or defined from star imports(F405)
1519-1519:
timedeltamay be undefined, or defined from star imports(F405)
1522-1522:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1526-1526:
timedeltamay be undefined, or defined from star imports(F405)
1529-1529:
timedeltamay be undefined, or defined from star imports(F405)
1532-1532:
create_ap_paymentmay be undefined, or defined from star imports(F405)
apps/sage_intacct/tasks.py
1100-1102: Use a single
ifstatement instead of nestedifstatements(SIM102)
1253-1255: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (12)
tests/test_sageintacct/test_tasks.py (1)
1442-1534: LGTM!The new test function
test_skipping_create_ap_paymentthoroughly tests the skipping behavior ofcreate_ap_paymentbased on different time ranges of the associatedTaskLog. It covers scenarios when theTaskLogwas created more than 2 months ago, between 1 and 2 months ago, and within the last 1 month. For each scenario, it verifies that theupdated_attimestamp remains unchanged after callingcreate_ap_payment, indicating that the function skipped processing as expected.The test setup, mocking of dependencies, and assertions are all properly implemented.
Tools
Ruff
1487-1487:
create_billmay be undefined, or defined from star imports(F405)
1489-1489:
Billmay be undefined, or defined from star imports(F405)
1499-1499:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1501-1501:
timezonemay be undefined, or defined from star imports(F405)
1502-1502:
timedeltamay be undefined, or defined from star imports(F405)
1505-1505:
timedeltamay be undefined, or defined from star imports(F405)
1511-1511:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1516-1516:
timedeltamay be undefined, or defined from star imports(F405)
1519-1519:
timedeltamay be undefined, or defined from star imports(F405)
1522-1522:
create_ap_paymentmay be undefined, or defined from star imports(F405)
1526-1526:
timedeltamay be undefined, or defined from star imports(F405)
1529-1529:
timedeltamay be undefined, or defined from star imports(F405)
1532-1532:
create_ap_paymentmay be undefined, or defined from star imports(F405)
apps/sage_intacct/tasks.py (3)
1-1: Skipping comment on the TODO as it's a duplicate from a previous review.
1122-1127: LGTM!The code changes to skip creating AP payment based on the validation function are approved.
1275-1280: LGTM!The code changes to skip creating reimbursement based on the validation function are approved.
apps/sage_intacct/models.py (2)
523-523: LGTM!The new
is_retiredfield with default valueFalseis added correctly to theBillmodel. The help text provides clarity on the purpose of the field.
714-714: LGTM!The new
is_retiredfield with default valueFalseis added correctly to theExpenseReportmodel. The help text provides clarity on the purpose of the field.tests/sql_fixtures/reset_db_fixtures/reset_db.sql (6)
280-281: LGTM!The addition of the
is_retiredcolumn to thebillstable is a valid enhancement to the data model.
1108-1109: LGTM!The addition of the
is_retiredcolumn to theexpense_reportstable is a valid enhancement to the data model.
2849-2849: LGTM!The update to the
COPYcommand for thebillstable is necessary to populate the newis_retiredcolumn.
4125-4125: LGTM!The update to the
django_migrationssequence value is valid and reflects the latest migration.
7554-7554: LGTM!The update to the
COPYcommand for theexpense_reportstable is necessary to populate the newis_retiredcolumn.
8133-8133: LGTM!The update to the
django_migrations_id_seqsequence value is valid and reflects the latest migration.
| now = datetime.now().replace(tzinfo=timezone.utc) | ||
| updated_at = now - timedelta(days=10) |
There was a problem hiding this comment.
Fix undefined names.
The names timezone and timedelta are undefined or imported from star imports. Please explicitly import them to avoid potential issues.
+from datetime import timedelta
+from django.utils import timezoneAlso applies to: 516-517, 1501-1502, 1505-1505, 1516-1516, 1519-1519, 1526-1526, 1529-1529
Tools
Ruff
458-458:
timezonemay be undefined, or defined from star imports(F405)
459-459:
timedeltamay be undefined, or defined from star imports(F405)
| reimbursement.state = 'COMPLETE' | ||
| reimbursement.save() | ||
|
|
||
| create_bill(expense_group, task_log.id, True) |
There was a problem hiding this comment.
Fix undefined names.
The names create_bill, Bill, and create_ap_payment are undefined or imported from star imports. Please explicitly import them to avoid potential issues.
+from apps.sage_intacct.models import Bill
+from apps.sage_intacct.tasks import create_bill, create_ap_paymentAlso applies to: 1489-1489, 1499-1499, 1511-1511, 1522-1522, 1532-1532
Tools
Ruff
1487-1487:
create_billmay be undefined, or defined from star imports(F405)
| @@ -1,3 +1,6 @@ | |||
| from datetime import datetime | |||
| from dateutil.relativedelta import relativedelta | |||
There was a problem hiding this comment.
Remove unused import.
The import dateutil.relativedelta.relativedelta is not used in the code. Please remove it to keep the code clean.
-from dateutil.relativedelta import relativedeltaCommittable 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.
| from dateutil.relativedelta import relativedelta |
Tools
Ruff
2-2:
dateutil.relativedelta.relativedeltaimported but unusedRemove unused import:
dateutil.relativedelta.relativedelta(F401)
apps/sage_intacct/tasks.py
Outdated
| def validate_for_skipping_payment(expense_group_id, workspace_id): | ||
| task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(expense_group_id), workspace_id=workspace_id, type='CREATING_AP_PAYMENT').first() | ||
| if task_log: | ||
| now = timezone.now() | ||
|
|
||
| if now - relativedelta(months=2) > task_log.created_at: | ||
| return True | ||
|
|
||
| elif now - relativedelta(months=1) > task_log.created_at and now - relativedelta(months=2) < task_log.created_at: | ||
| # if updated_at is within 1 months will be skipped | ||
| if task_log.updated_at > now - relativedelta(months=1): | ||
| return True | ||
|
|
||
| # If created is within 1 month | ||
| elif now - relativedelta(months=1) < task_log.created_at: | ||
| # Skip if updated within the last week | ||
| if task_log.updated_at > now - relativedelta(weeks=1): | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
Refactor the nested if-else statements to simplify the logic.
Consider flattening the nested if-else statements to improve readability.
Here's a suggested refactor:
def validate_for_skipping_payment(expense_group_id, workspace_id):
task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(expense_group_id), workspace_id=workspace_id, type='CREATING_AP_PAYMENT').first()
if task_log:
now = timezone.now()
two_months_ago = now - relativedelta(months=2)
one_month_ago = now - relativedelta(months=1)
one_week_ago = now - relativedelta(weeks=1)
if task_log.created_at < two_months_ago:
return True
elif one_month_ago < task_log.created_at < two_months_ago and task_log.updated_at > one_month_ago:
return True
elif task_log.created_at > one_month_ago and task_log.updated_at > one_week_ago:
return True
return FalseTools
Ruff
1100-1102: Use a single
ifstatement instead of nestedifstatements(SIM102)
apps/sage_intacct/tasks.py
Outdated
| def validate_for_skipping_reimbursement(expense_group_id, workspace_id): | ||
|
|
||
| task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(expense_group_id), workspace_id=workspace_id, type='CREATING_REIMBURSEMENT').first() | ||
| if task_log: | ||
| now = timezone.now() | ||
|
|
||
| if now - relativedelta(months=2) > task_log.created_at: | ||
| return True | ||
|
|
||
| elif now - relativedelta(months=1) > task_log.created_at and now - relativedelta(months=2) < task_log.created_at: | ||
| # if updated_at is within 1 months will be skipped | ||
| if task_log.updated_at > now - relativedelta(months=1): | ||
| return True | ||
|
|
||
| # If created is within 1 month | ||
| elif now - relativedelta(months=1) < task_log.created_at: | ||
| # Skip if updated within the last week | ||
| if task_log.updated_at > now - relativedelta(weeks=1): | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
Refactor the nested if-else statements to simplify the logic.
Consider flattening the nested if-else statements to improve readability, similar to the refactor suggested for the validate_for_skipping_payment function.
Here's a suggested refactor:
def validate_for_skipping_reimbursement(expense_group_id, workspace_id):
task_log = TaskLog.objects.filter(task_id='PAYMENT_{}'.format(expense_group_id), workspace_id=workspace_id, type='CREATING_REIMBURSEMENT').first()
if task_log:
now = timezone.now()
two_months_ago = now - relativedelta(months=2)
one_month_ago = now - relativedelta(months=1)
one_week_ago = now - relativedelta(weeks=1)
if task_log.created_at < two_months_ago:
return True
elif one_month_ago < task_log.created_at < two_months_ago and task_log.updated_at > one_month_ago:
return True
elif task_log.created_at > one_month_ago and task_log.updated_at > one_week_ago:
return True
return FalseTools
Ruff
1253-1255: Use a single
ifstatement instead of nestedifstatements(SIM102)
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/sage_intacct/tasks.py (3 hunks)
Additional comments not posted (3)
apps/sage_intacct/tasks.py (3)
1086-1107: LGTM!The code changes are approved.
1116-1116: LGTM!The code changes are approved.
Also applies to: 1125-1127
1245-1245: LGTM!The code changes are approved.
Also applies to: 1253-1255
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (5)
apps/sage_intacct/models.py (3)
568-568: Approved, but consider renaming the field for clarity.The addition of the
is_retiredfield to track payment sync retry attempts is a good feature. However, the field name might be slightly misleading. Consider renaming it to something more explicit likepayment_sync_retriedorhas_retry_attemptto better reflect its purpose.- is_retired = models.BooleanField(help_text='Is Payment sync retried', default=False) + payment_sync_retried = models.BooleanField(help_text='Has payment sync been retried', default=False)
761-761: Approved, but consider renaming the field for clarity and consistency.The addition of the
is_retiredfield to theExpenseReportmodel is consistent with the change made to theBillmodel, which is good. However, as suggested for theBillmodel, consider renaming this field to something more explicit likepayment_sync_retriedorhas_retry_attempt. This will make the purpose of the field clearer and maintain consistency across both models.- is_retired = models.BooleanField(help_text='Is Payment sync retried', default=False) + payment_sync_retried = models.BooleanField(help_text='Has payment sync been retried', default=False)
Line range hint
1-1452: Summary of changes and potential impactThe changes made to this file are focused and consistent, adding an
is_retiredfield to both theBillandExpenseReportmodels. These additions appear to be part of implementing a feature to track payment sync retry attempts, which aligns with the PR objective of implementing a maximum retry limit for payment processing.While the changes themselves are straightforward and unlikely to cause issues, consider the following:
- The suggested renaming of the field to
payment_sync_retriedor similar would improve code readability and maintainability.- Ensure that any code interacting with these models is updated to use the new field appropriately.
- Consider adding a database migration file to apply these changes to the database schema.
- Update any relevant documentation to reflect this new feature for tracking retry attempts.
Overall, these changes provide a good foundation for implementing the retry limit functionality, enhancing the reliability of the payment processing system.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
280-281: LGTM! Consider adding a default value for existing records.The addition of the
is_retiredcolumn to thebillstable is appropriate and aligns with the PR objectives. The boolean type and NOT NULL constraint are suitable for this flag.Consider adding a DEFAULT value (e.g.,
DEFAULT FALSE) to ensure existing records are properly handled during the migration:is_retired boolean NOT NULL DEFAULT FALSE
1111-1112: LGTM! Consider adding a default value for existing records.The addition of the
is_retiredcolumn to theexpense_reportstable is appropriate and consistent with the changes made to thebillstable.Consider adding a DEFAULT value (e.g.,
DEFAULT FALSE) to ensure existing records are properly handled during the migration:is_retired boolean NOT NULL DEFAULT FALSE
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/sage_intacct/migrations/0029_auto_20240902_1511.py (1 hunks)
- apps/sage_intacct/models.py (2 hunks)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql (6 hunks)
Additional comments not posted (5)
apps/sage_intacct/migrations/0029_auto_20240902_1511.py (1)
6-10: LGTM: Migration class and dependencies are correctly defined.The migration class is properly structured and the dependency on the previous migration '0028_add_billable_field_to_cct' is correctly specified. This ensures that the migrations will be applied in the right order.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
2852-2852: LGTM! COPY command updated correctly.The COPY command for the
billstable has been properly updated to include the newis_retiredcolumn, maintaining consistency with the table structure.
7561-7561: LGTM! COPY command updated correctly.The COPY command for the
expense_reportstable has been properly updated to include the newis_retiredcolumn, maintaining consistency with the table structure.
8140-8140: LGTM! Sequence value updated correctly.The sequence value for
django_migrations_id_seqhas been properly updated to 195, which is consistent with the addition of the new migration entry.
4132-4132: LGTM! Verify corresponding migration file.The new migration entry for
sage_intacct(0029_auto_20240902_1511) has been correctly added to thedjango_migrationstable.Please ensure that the corresponding migration file exists and contains the changes for adding the
is_retiredcolumn to bothbillsandexpense_reportstables.Verification successful
Migration File Verified Successfully
The migration file
apps/sage_intacct/migrations/0029_auto_20240902_1511.pyexists and correctly adds theis_retiredcolumn to both thebillandexpense_reportstables.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the migration file # Test: Check if the migration file exists fd --type f "0029_auto_20240902_1511.py" apps/sage_intacct/migrations # Test: Display the content of the migration file fd --type f "0029_auto_20240902_1511.py" apps/sage_intacct/migrations --exec catLength of output: 847
| migrations.AddField( | ||
| model_name='bill', | ||
| name='is_retired', | ||
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | ||
| ), |
There was a problem hiding this comment.
Consider renaming the 'is_retired' field for clarity.
The field name 'is_retired' doesn't accurately represent its purpose based on the provided help text. The help text suggests this field is about retrying payment sync, not retirement.
Consider renaming the field to better reflect its purpose. For example:
- name='is_retired',
+ name='is_payment_sync_retried',
field=models.BooleanField(default=False, help_text='Is Payment sync retried'),This change would make the field name consistent with its intended use and improve code readability.
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.
| migrations.AddField( | |
| model_name='bill', | |
| name='is_retired', | |
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | |
| ), | |
| migrations.AddField( | |
| model_name='bill', | |
| name='is_payment_sync_retried', | |
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | |
| ), |
| migrations.AddField( | ||
| model_name='expensereport', | ||
| name='is_retired', | ||
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | ||
| ), |
There was a problem hiding this comment.
Rename 'is_retired' field in 'expensereport' model for consistency and clarity.
Similar to the 'bill' model, the field name 'is_retired' in the 'expensereport' model doesn't accurately represent its purpose based on the provided help text.
For consistency, rename this field as well:
- name='is_retired',
+ name='is_payment_sync_retried',
field=models.BooleanField(default=False, help_text='Is Payment sync retried'),Ensure that corresponding changes are made in the models.py file and any other relevant parts of the codebase where these fields are referenced.
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.
| migrations.AddField( | |
| model_name='expensereport', | |
| name='is_retired', | |
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | |
| ), | |
| migrations.AddField( | |
| model_name='expensereport', | |
| name='is_payment_sync_retried', | |
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | |
| ), |
| # Generated by Django 3.2.14 on 2024-09-02 15:11 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('sage_intacct', '0028_add_billable_field_to_cct'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name='bill', | ||
| name='is_retired', | ||
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='expensereport', | ||
| name='is_retired', | ||
| field=models.BooleanField(default=False, help_text='Is Payment sync retried'), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
The is_retired field is used in multiple locations and should be renamed for consistency.
The field is_retired appears in the following files:
apps/sage_intacct/tasks.pyapps/sage_intacct/migrations/0029_auto_20240902_1511.pyapps/sage_intacct/models.py
Please rename the is_retired field across all these files to ensure consistent naming conventions throughout the codebase.
Analysis chain
Overall, the migration is structurally correct but requires field renaming.
The migration file correctly adds new boolean fields to the 'bill' and 'expensereport' models. However, the field names ('is_retired') are inconsistent with their intended purpose as described in the help text.
To ensure consistency across the codebase, please run the following script to check for any other occurrences of 'is_retired' that might need updating:
After renaming the fields:
- Update this migration file.
- Modify the corresponding model definitions in models.py.
- Update any references to these fields in views, serializers, and other relevant parts of the codebase.
- Consider adding a comment in the models explaining the purpose of these fields to prevent future confusion.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for 'is_retired' occurrences in Python files
# Test: Search for 'is_retired' in Python files
rg --type python 'is_retired'
Length of output: 60
Script:
#!/bin/bash
# Description: Search for 'is_retired' occurrences in Python files using the correct file type
rg --type py 'is_retired'
Length of output: 686
* Max retry payment Limit with test * updated test * Creating reimbursement max retry limit * comment resolved
Summary by CodeRabbit
New Features
is_retiredin bills and expense reports to track payment synchronization states.Bug Fixes
Tests