Conversation
ashwin1111
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughSwitched ExpenseGroupSettings signal handling from pre_save to post_save in apps/fyle/signals.py, renaming the receiver accordingly and updating imports and docstrings. Core logic remains, now executed after model save. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Model as ExpenseGroupSettings (Model)
participant ORM as Django ORM
participant Signal as post_save Receiver
User->>Model: create/update instance
Model->>ORM: save()
ORM-->>Model: persist changes
Note over ORM,Signal: post_save emitted after successful save
ORM-->>Signal: post_save(sender=ExpenseGroupSettings, instance)
activate Signal
Signal->>Signal: run_post_save_expense_group_setting_triggers(instance)
alt Conditions met
Signal-->>ORM: trigger related actions
else Conditions not met
Signal-->>ORM: no-op
end
deactivate Signal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/fyle/signals.py (1)
21-35: Logic is unreachable due to post_save timing.The existence check on line 25 (
if existing_expense_group_setting:) will always beTruein apost_savecontext becauseinstancewas just saved. More critically, the state-change comparisons on lines 29 and 33 will always evaluate toFalsebecause bothexisting_expense_group_settingandinstancereference the same saved state.Consequence: The async expense pull tasks on lines 31 and 35 will never execute, breaking the configuration-triggered expense import functionality described in the PR title ("fix: update configuration <> expense pull").
This confirms the critical issue flagged in the previous comment: the signal handler must be reverted to
pre_saveor substantially refactored to work withpost_save.Note: The business logic itself (checking for PAID → PAYMENT_PROCESSING and PAID → APPROVED transitions) appears correct and well-structured, but it's incompatible with the current signal timing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/fyle/signals.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/fyle/signals.py (2)
apps/fyle/enums.py (1)
FundSourceEnum(6-11)apps/fyle/models.py (1)
ExpenseGroupSettings(295-445)
🪛 Ruff (0.14.0)
apps/fyle/signals.py
17-17: Unused function argument: sender
(ARG001)
17-17: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (1)
apps/fyle/signals.py (1)
3-6: Verify library enum definitions in apps/fyle/signals.py lines 3–6
Ensure thatFundSourceEnumandExpenseImportSourceEnumimported fromfyle_accounting_library.fyle_platform.enumsmatch your local enum values (e.g.PERSONAL,CCC) to prevent runtime mismatches.
| @receiver(post_save, sender=ExpenseGroupSettings) | ||
| def run_post_save_expense_group_setting_triggers(sender, instance: ExpenseGroupSettings, **kwargs): |
There was a problem hiding this comment.
Critical: post_save breaks change detection logic.
Switching from pre_save to post_save fundamentally breaks the change-detection logic on lines 29 and 33. In a post_save signal:
instanceis the already-saved object with new values- Querying the database (lines 21-23) returns the same object with the same new values
- Comparisons like
existing_expense_group_setting.reimbursable_expense_state != instance.reimbursable_expense_statewill always be False - The async tasks on lines 31 and 35 will never be triggered
In pre_save, the logic worked because:
instancehad the new values (not yet saved)- The database query returned the old values
- The comparison detected actual changes
Solution options:
Option 1 (recommended): Revert to pre_save if there's no compelling reason for post_save.
-from django.db.models.signals import post_save
+from django.db.models.signals import pre_save
from django.dispatch import receiver-@receiver(post_save, sender=ExpenseGroupSettings)
-def run_post_save_expense_group_setting_triggers(sender, instance: ExpenseGroupSettings, **kwargs):
+@receiver(pre_save, sender=ExpenseGroupSettings)
+def run_pre_save_expense_group_setting_triggers(sender, instance: ExpenseGroupSettings, **kwargs):
"""
- Run post save expense group setting triggers
+ Run pre save expense group setting triggers
"""Option 2: If post_save is required, track the previous state using update_fields:
@receiver(post_save, sender=ExpenseGroupSettings)
def run_post_save_expense_group_setting_triggers(sender, instance: ExpenseGroupSettings, **kwargs):
"""
Run post save expense group setting triggers
"""
+ # Skip if this is a new instance
+ if kwargs.get('created', False):
+ return
+
+ # Only proceed if reimbursable_expense_state or ccc_expense_state were updated
+ update_fields = kwargs.get('update_fields', None)
+ if update_fields and not {'reimbursable_expense_state', 'ccc_expense_state'} & set(update_fields):
+ return
+
- existing_expense_group_setting = ExpenseGroupSettings.objects.filter(
- workspace_id=instance.workspace_id
- ).first()
+ # Retrieve the old state from the database before the save
+ # Note: This won't work in post_save as shown - you'd need to cache old values in __init__
+ # or use a django-dirtyfields library or similar approachNote: Option 2 requires additional refactoring to properly track previous state. The update_fields check alone isn't sufficient because you still need the old values for comparison.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.0)
17-17: Unused function argument: sender
(ARG001)
17-17: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
apps/fyle/signals.py lines 16-17: switching the signal to post_save breaks
change-detection because instance and DB both reflect new values so comparisons
never detect changes; restore the original behavior by reverting the decorator
to pre_save so instance contains new values and DB returns old values for
correct diffing, or if post_save is mandatory, add a pre_save handler that
snapshots the existing DB state onto the instance (e.g.,
instance._pre_save_snapshot = ExpenseGroupSettings.objects.get(pk=instance.pk)
when pk exists) and then in the post_save handler compare instance against
instance._pre_save_snapshot to trigger the async tasks.
2 participants
https://app.clickup.com/