fix: notify_webhooks break instead of continue#3396
Open
Manishnemade12 wants to merge 7 commits intofossasia:devfrom
Open
fix: notify_webhooks break instead of continue#3396Manishnemade12 wants to merge 7 commits intofossasia:devfrom
Manishnemade12 wants to merge 7 commits intofossasia:devfrom
Conversation
- Replace break with continue in notify_webhooks Celery task when a log entry lacks an organizer or a matching webhook_type - Add logger.debug statements when skipping these entries - Using break caused the entire batch of log entries to be silently dropped if any single entry in the batch triggered these conditions
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the notify_webhooks Celery task so that invalid log entries are skipped rather than aborting the entire batch, and adds debug logging to make skipped entries observable. Sequence diagram for notify_webhooks batch processing with skipping invalid log entriessequenceDiagram
participant CeleryWorker
participant WebhookNotifier
participant LogEntry
participant OrganizerWebhooks
participant Logger
CeleryWorker->>WebhookNotifier: notify_webhooks(logentry_ids)
WebhookNotifier->>WebhookNotifier: load queryset qs from logentry_ids
loop for each logentry in qs
WebhookNotifier->>LogEntry: read organizer
alt organizer is missing
WebhookNotifier->>Logger: debug(Skipping: no organizer)
WebhookNotifier-->>WebhookNotifier: continue to next logentry
else organizer present
WebhookNotifier->>LogEntry: read webhook_type
alt webhook_type is missing
WebhookNotifier->>Logger: debug(Skipping: no matching webhook event type)
WebhookNotifier-->>WebhookNotifier: continue to next logentry
else webhook_type present
WebhookNotifier->>OrganizerWebhooks: fetch webhooks for organizer and action_type
OrganizerWebhooks-->>WebhookNotifier: webhooks list
WebhookNotifier->>OrganizerWebhooks: send_notifications(webhooks, logentry)
end
end
end
WebhookNotifier-->>CeleryWorker: return after processing all logentries
Flow diagram for notify_webhooks log entry filtering and processingflowchart TD
A[Start notify_webhooks
logentry_ids] --> B[Load queryset qs]
B --> C{More logentries?}
C -->|No| Z[End task]
C -->|Yes| D[Get next logentry]
D --> E{logentry.organizer exists?}
E -->|No| E1[logger.debug
Skipping: no organizer]
E1 --> C
E -->|Yes| F{logentry.webhook_type exists?}
F -->|No| F1[logger.debug
Skipping: no matching webhook event type]
F1 --> C
F -->|Yes| G[Check cached
_org, _at, webhooks]
G --> H[Fetch or reuse
webhooks for organizer
and action_type]
H --> I[Send webhook
notifications]
I --> C
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider including additional context (e.g., organizer ID or event ID) in the debug messages so that skipped log entries can be traced more easily during debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider including additional context (e.g., organizer ID or event ID) in the debug messages so that skipped log entries can be traced more easily during debugging.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes webhook notification batching so that individual malformed/unhandled log entries no longer stop processing of the remaining entries in the same Celery task run.
Changes:
- Replace
breakwithcontinueinnotify_webhookswhen aLogEntryhas no organizer. - Replace
breakwithcontinueinnotify_webhookswhen aLogEntryhas no matchingwebhook_type. - Add
logger.debugmessages when entries are skipped for these reasons.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment on lines
269
to
+274
| if not logentry.organizer: | ||
| break # We need to know the organizer | ||
| logger.debug( | ||
| 'Skipping webhook notification for log entry %d: no organizer', | ||
| logentry.id, | ||
| ) | ||
| continue # We need to know the organizer, skip this entry |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Closes : #3400
Summary by Sourcery
Ensure webhook notification processing skips invalid log entries instead of aborting the entire batch.
Bug Fixes:
Enhancements: