feat: system comments for p1 areas #900
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Contributor
WalkthroughCentralizes system comment collection across Sage Intacct export flows, adds billable line-item validation that emits system comments when required dimensions are missing, introduces new system-comment enum members, adds tests and fixtures for these behaviors, and adds a DB migration to run raw SQL. Changes
Sequence Diagram(s)sequenceDiagram
participant Export as Export Function
participant Validator as Billable Validator
participant SystemSvc as SystemComment Service
participant External as Intacct API
participant DB as Database
Export->>SystemSvc: init system_comments = []
loop each expense_group / lineitem
Export->>Validator: check billable dimensions
alt missing customer_id/item_id
Validator->>SystemSvc: append comment(source,intent,reason,info)
end
Export->>External: prepare/send payload
alt API error or export-state condition
External-->>Export: error/state
Export->>SystemSvc: append error/skip/retrigger comment
end
end
alt system_comments not empty
SystemSvc->>DB: bulk_create_comments(system_comments)
DB-->>SystemSvc: persisted
end
Export->>Export: finish flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Failure. Coverage is below 90%.Diff Coverage
|
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/sage_intacct/models.py (1)
1129-1167: Keep the billable flag consistent with the dimensions you persistThe new warning uses
dimensions_values(post-allocation), but the savedbillableflag still uses the pre-allocationcustomer_id/item_idvariables. If allocation clears those dimensions, you can persistbillable=Truewhilecustomer_id/item_idare NULL, yet log “billable disabled.” Compute billable once from the samedimensions_valuesand reuse it here (and in the other lineitem creators).Proposed fix
- if lineitem.billable and not (dimensions_values['customer_id'] and dimensions_values['item_id']): + is_billable = bool(lineitem.billable and dimensions_values['customer_id'] and dimensions_values['item_id']) + if lineitem.billable and not is_billable: missing_fields = [] if not dimensions_values['customer_id']: missing_fields.append('customer_id') if not dimensions_values['item_id']: missing_fields.append('item_id') add_system_comment( system_comments=system_comments, source=SystemCommentSourceEnum.CREATE_BILL_LINEITEMS, intent=SystemCommentIntentEnum.BILLABLE_DISABLED, entity_type=SystemCommentEntityTypeEnum.EXPENSE, workspace_id=expense_group.workspace_id, entity_id=lineitem.id, reason=SystemCommentReasonEnum.BILLABLE_SET_TO_FALSE_MISSING_DIMENSIONS, info={'missing_fields': missing_fields, 'original_billable': lineitem.billable} ) bill_lineitem_object, _ = BillLineitem.objects.update_or_create( bill=bill, expense_id=lineitem.id, defaults={ @@ - 'billable': lineitem.billable if customer_id and item_id else False, + 'billable': is_billable,apps/sage_intacct/tasks.py (1)
891-963: Missingsystem_commentsbulk creation in journal entry exception handlers.Unlike
create_expense_report,create_bill, andcreate_charge_card_transaction, thecreate_journal_entryfunction does not persistsystem_commentsin its exception handlers. This means any collected comments (e.g., from lines 750-760 for ERROR state or 789-798 for fallback vendor) will be lost if the export fails.🐛 Proposed fix - add bulk create to each exception handler
For example, in the
BulkErrorhandler:except BulkError as exception: + if system_comments: + for comment in system_comments: + comment['workspace_id'] = expense_group.workspace_id + comment['export_type'] = ExportTypeEnum.JOURNAL_ENTRY + SystemComment.bulk_create_comments(system_comments) logger.info(exception.response) detail = exception.responseApply the same pattern to all other exception handlers (
WrongParamsError,NoPrivilegeError,InvalidTokenError,ValueErrorWithResponse,IntacctRESTBadRequestError,IntacctRESTInvalidTokenError,IntacctRESTInternalServerError, andException).
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 350-372: The fixture get_or_create_task_log creates TaskLog
instances but doesn't apply a provided updated_at because TaskLog.updated_at
uses auto_now=True which ignores create() values; modify the
_get_or_create_task_log function so that if updated_at is provided and a TaskLog
was just created (or exists), set task_log.updated_at = updated_at and call
task_log.save(update_fields=["updated_at"]) (referencing TaskLog and
_get_or_create_task_log) to persist the historical timestamp used by tests.
In `@tests/test_system_comments/test_system_comments.py`:
- Around line 993-998: The mock for Schedule.objects.filter needs to support
chaining to .first()—instead of returning None from .filter(), have the patched
return value provide a .first() method (e.g. return a MagicMock whose first()
returns None or the desired object); update the mocker.patch call that targets
Schedule.objects.filter so it returns a MagicMock with first configured
(referencing Schedule.objects.filter and its .first() call) so the test no
longer raises AttributeError.
🧹 Nitpick comments (2)
apps/sage_intacct/tasks.py (2)
1095-1099: Consider extracting repeated system comment persistence logic into a helper.The same 5-line block for persisting system comments is repeated in the success path and every exception handler across
create_expense_report,create_bill, andcreate_charge_card_transaction(30+ times total). This could be simplified with a helper function.♻️ Suggested helper function
def persist_system_comments(system_comments: list, workspace_id: int, export_type: ExportTypeEnum) -> None: """Persist collected system comments with workspace and export type context.""" if system_comments: for comment in system_comments: comment['workspace_id'] = workspace_id comment['export_type'] = export_type SystemComment.bulk_create_comments(system_comments)Then each handler becomes a single call:
persist_system_comments(system_comments, expense_group.workspace_id, ExportTypeEnum.EXPENSE_REPORT)
1852-1852: Type hint should use explicit| Noneinstead of implicitOptional.Per PEP 484 and the static analysis hint (RUF013), the parameter type should explicitly indicate it can be
None.-def validate_for_skipping_payment(export_module: Bill | ExpenseReport, workspace_id: int, type: str, system_comments: list = None) -> bool: +def validate_for_skipping_payment(export_module: Bill | ExpenseReport, workspace_id: int, type: str, system_comments: list | None = None) -> bool:
Comment on lines
+350
to
+372
| @pytest.fixture | ||
| def get_or_create_task_log(db) -> Callable: | ||
| """ | ||
| Fixture to get or create a TaskLog for an expense group | ||
| Returns a function that can be called with expense_group and optional parameters | ||
| """ | ||
| def _get_or_create_task_log( | ||
| expense_group: ExpenseGroup, | ||
| task_type: str = 'FETCHING_EXPENSES', | ||
| status: str = 'COMPLETE', | ||
| updated_at: Optional[datetime] = None | ||
| ) -> TaskLog: | ||
| task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first() | ||
| if not task_log: | ||
| task_log = TaskLog.objects.create( | ||
| expense_group_id=expense_group.id, | ||
| workspace_id=expense_group.workspace_id, | ||
| type=task_type, | ||
| status=status, | ||
| updated_at=updated_at | ||
| ) | ||
| return task_log | ||
| return _get_or_create_task_log |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "class TaskLog" | head -5Repository: fylein/fyle-intacct-api
Length of output: 117
🏁 Script executed:
cat ./apps/tasks/models.pyRepository: fylein/fyle-intacct-api
Length of output: 6929
🏁 Script executed:
grep -n "get_or_create_task_log" tests/conftest.py -A 5 -B 2Repository: fylein/fyle-intacct-api
Length of output: 642
🏁 Script executed:
grep -r "get_or_create_task_log" tests/ --include="*.py" -nRepository: fylein/fyle-intacct-api
Length of output: 7416
🏁 Script executed:
sed -n '976,1000p' tests/test_system_comments/test_system_comments.pyRepository: fylein/fyle-intacct-api
Length of output: 1206
🏁 Script executed:
sed -n '1756,1775p' tests/test_system_comments/test_system_comments.pyRepository: fylein/fyle-intacct-api
Length of output: 734
Handle updated_at after TaskLog creation since auto_now=True prevents passing it to create()
TaskLog.updated_at uses auto_now=True, so values passed to create() are silently ignored. Tests relying on historical timestamps (e.g., stuck exports detection) will use current time instead. Update the timestamp after creation when provided.
def _get_or_create_task_log(
expense_group: ExpenseGroup,
task_type: str = 'FETCHING_EXPENSES',
status: str = 'COMPLETE',
updated_at: Optional[datetime] = None
) -> TaskLog:
task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first()
if not task_log:
task_log = TaskLog.objects.create(
expense_group_id=expense_group.id,
workspace_id=expense_group.workspace_id,
type=task_type,
status=status,
updated_at=updated_at
)
+ if updated_at is not None:
+ TaskLog.objects.filter(id=task_log.id).update(updated_at=updated_at)
+ task_log.refresh_from_db()
return task_log🧰 Tools
🪛 Ruff (0.14.14)
351-351: Unused function argument: db
(ARG001)
🤖 Prompt for AI Agents
In `@tests/conftest.py` around lines 350 - 372, The fixture get_or_create_task_log
creates TaskLog instances but doesn't apply a provided updated_at because
TaskLog.updated_at uses auto_now=True which ignores create() values; modify the
_get_or_create_task_log function so that if updated_at is provided and a TaskLog
was just created (or exists), set task_log.updated_at = updated_at and call
task_log.save(update_fields=["updated_at"]) (referencing TaskLog and
_get_or_create_task_log) to persist the historical timestamp used by tests.
Comment on lines
+993
to
+998
| captured_comments = [] | ||
| mocker.patch('apps.internal.tasks.export_to_intacct') | ||
| mocker.patch('apps.internal.tasks.update_failed_expenses') | ||
| mocker.patch('apps.internal.tasks.post_accounting_export_summary') | ||
| mocker.patch('apps.internal.tasks.Schedule.objects.filter', return_value=mocker.MagicMock(filter=mocker.MagicMock(return_value=None))) | ||
| mocker.patch.object(SystemComment, 'bulk_create_comments', side_effect=lambda comments: captured_comments.extend(comments)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file and the surrounding context
cat -n tests/test_system_comments/test_system_comments.py | sed -n '990,1010p'Repository: fylein/fyle-intacct-api
Length of output: 1401
🏁 Script executed:
# Let's get more context around the test function
cat -n tests/test_system_comments/test_system_comments.py | sed -n '970,1020p'Repository: fylein/fyle-intacct-api
Length of output: 2910
🏁 Script executed:
# Let's find the test function name to understand what's being tested
cat -n tests/test_system_comments/test_system_comments.py | sed -n '950,1000p'Repository: fylein/fyle-intacct-api
Length of output: 2951
🏁 Script executed:
# Find the retrigger_stuck_exports function
rg -n "def retrigger_stuck_exports" --type pyRepository: fylein/fyle-intacct-api
Length of output: 131
🏁 Script executed:
# Look for usage of Schedule.objects.filter in the codebase
rg -n "Schedule\.objects\.filter" --type py -A 3Repository: fylein/fyle-intacct-api
Length of output: 18382
🏁 Script executed:
# Get the retrigger_stuck_exports function implementation
cat -n apps/internal/tasks.py | sed -n '38,200p'Repository: fylein/fyle-intacct-api
Length of output: 8262
🏁 Script executed:
# Verify the test actually fails with current mock by checking if there are any other test runs
# Also check if this is the only place this mock pattern appears
rg -n "Schedule.objects.filter.*return_value.*MagicMock.*filter.*MagicMock.*return_value.*None" tests/Repository: fylein/fyle-intacct-api
Length of output: 260
🏁 Script executed:
# Check line 1777 which also has a Schedule.objects.filter mock
cat -n tests/test_system_comments/test_system_comments.py | sed -n '1770,1800p'Repository: fylein/fyle-intacct-api
Length of output: 1897
Fix mock to support .filter(...).first() chaining
The current mock returns None from .filter(), which causes .first() to raise AttributeError. Set up the mock to properly chain these calls.
Proposed fix
- mocker.patch('apps.internal.tasks.Schedule.objects.filter', return_value=mocker.MagicMock(filter=mocker.MagicMock(return_value=None)))
+ mock_schedule_qs = mocker.MagicMock()
+ mock_schedule_filter = mocker.MagicMock()
+ mock_schedule_filter.first.return_value = None
+ mock_schedule_qs.filter.return_value = mock_schedule_filter
+ mocker.patch('apps.internal.tasks.Schedule.objects.filter', return_value=mock_schedule_qs)🤖 Prompt for AI Agents
In `@tests/test_system_comments/test_system_comments.py` around lines 993 - 998,
The mock for Schedule.objects.filter needs to support chaining to
.first()—instead of returning None from .filter(), have the patched return value
provide a .first() method (e.g. return a MagicMock whose first() returns None or
the desired object); update the mocker.patch call that targets
Schedule.objects.filter so it returns a MagicMock with first configured
(referencing Schedule.objects.filter and its .first() call) so the test no
longer raises AttributeError.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/sage_intacct/tasks.py`:
- Around line 746-761: system_comments are being accumulated via
add_system_comment but the bulk-create is only executed on the success path of
the Intacct journal-entry export, so any exception after accumulation causes
comments to be lost; locate where system_comments is collected (look for
variables named system_comments and calls to add_system_comment in the
journal-entry export flow for expense_group) and move the
persistence/bulk-create call for those system_comments into a finally block (or
ensure it's executed in finally) so comments are always saved regardless of
success or exception; apply the same change to the other similar blocks
referenced (around the ranges that add_system_comment at the other spots).
Comment on lines
746
to
+761
| in_progress_expenses = [] | ||
| system_comments = [] | ||
| # Don't include expenses with previous export state as ERROR and it's an auto import/export run | ||
| if not (is_auto_export and expense_group.expenses.first().previous_export_state == 'ERROR'): | ||
| if is_auto_export and expense_group.expenses.first().previous_export_state == 'ERROR': | ||
| add_system_comment( | ||
| system_comments=system_comments, | ||
| source=SystemCommentSourceEnum.CREATE_JOURNAL_ENTRY, | ||
| intent=SystemCommentIntentEnum.EXPORT_SUMMARY_NOT_UPDATED, | ||
| entity_type=SystemCommentEntityTypeEnum.EXPENSE_GROUP, | ||
| workspace_id=expense_group.workspace_id, | ||
| entity_id=expense_group.id, | ||
| export_type=ExportTypeEnum.JOURNAL_ENTRY, | ||
| reason=SystemCommentReasonEnum.EXPORT_SUMMARY_NOT_UPDATED_ERROR_STATE, | ||
| info={'previous_export_state': 'ERROR', 'is_auto_export': True} | ||
| ) | ||
| else: |
Contributor
There was a problem hiding this comment.
Persist system comments even when journal-entry export fails.
system_comments can be populated before the Intacct post, but the bulk-create runs only on the success path. Any exception after comment accumulation drops those comments (unlike other export flows that use finally). Please move the bulk-create to a finally block so comments are always persisted.
🛠️ Proposed fix
- if system_comments:
- for comment in system_comments:
- comment['workspace_id'] = expense_group.workspace_id
- comment['export_type'] = ExportTypeEnum.JOURNAL_ENTRY
- SystemComment.bulk_create_comments(system_comments)
+ finally:
+ if system_comments:
+ for comment in system_comments:
+ comment['workspace_id'] = expense_group.workspace_id
+ comment['export_type'] = ExportTypeEnum.JOURNAL_ENTRY
+ SystemComment.bulk_create_comments(system_comments)Also applies to: 789-798, 816-816, 857-862
🤖 Prompt for AI Agents
In `@apps/sage_intacct/tasks.py` around lines 746 - 761, system_comments are being
accumulated via add_system_comment but the bulk-create is only executed on the
success path of the Intacct journal-entry export, so any exception after
accumulation causes comments to be lost; locate where system_comments is
collected (look for variables named system_comments and calls to
add_system_comment in the journal-entry export flow for expense_group) and move
the persistence/bulk-create call for those system_comments into a finally block
(or ensure it's executed in finally) so comments are always saved regardless of
success or exception; apply the same change to the other similar blocks
referenced (around the ranges that add_system_comment at the other spots).
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff Coverage
|
Ashutosh619-sudo
approved these changes
Jan 28, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.