Feat: make some events webhook compatible#12936
Conversation
|
Thanks for opening your first pull request in this repository! ✌️ |
cfb6a98 to
de266d2
Compare
📝 WalkthroughWalkthroughThis PR adds webhook serialization support to message events. MessageDeletedEvent, MessageFlaggedEvent, and MessageSentEvent each implement 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/Events/MessageSentEvent.php (1)
40-43: ⚡ Quick winStabilize the webhook contract with an explicit payload schema.
Serializing
LocalMessagedirectly couples webhook output to internal entity shape and can expose more fields than intended. Prefer a curated flat payload (for example IDs + explicitly selected message fields).lib/Events/NewMessageReceivedEvent.php (1)
27-31: ⚡ Quick winAvoid emitting raw
Messageentities in webhook payloads.Returning the entity directly makes the external webhook schema implicit and fragile. Prefer an explicit, versionable payload with selected fields only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4a4f676-a8aa-41b8-8c4c-d5989f2ddac6
📒 Files selected for processing (5)
lib/Events/MessageDeletedEvent.phplib/Events/MessageFlaggedEvent.phplib/Events/MessageSentEvent.phplib/Events/NewMessageReceivedEvent.phplib/Listener/NewMessagesNotifier.php
8211490 to
d9199cc
Compare
|
@janepie is the target 34? |
Would be nice but I think I can live with 35, let me doublecheck |
Signed-off-by: Jana Peper <jana.peper@nextcloud.com>
Signed-off-by: Jana Peper <jana.peper@nextcloud.com>
0dd001e to
229f1a6
Compare
Co-authored-by: Daniel <mail@danielkesselberg.de> Signed-off-by: janepie <49834966+janepie@users.noreply.github.com>
229f1a6 to
4c6375e
Compare
|
Checked, 35 is fine! |
| return $this->set; | ||
| } | ||
|
|
||
| public function getWebhookSerializable(): array { |
There was a problem hiding this comment.
Do you have a use case in mind for this event?
I'm just wondering because you cannot use the messageUid for our api because the expects the message id (oc_mail_messages.id). The uid is an identifier on the imap server. The accountId and mailboxId are database ids from nextcloud mail.
mail/lib/Controller/MessageApiController.php
Line 234 in 3800491
With the uid you could directly lookup a message on the imap server, but for that you need the name of the mailbox (which you currently won't pass, only the id but that one the imap server wont know).
There was a problem hiding this comment.
Use case in mind is some hook that reacts whenever a message is flagged. Yeah I guess that would actually need the internal ID to do sth with it, tbh I just forwarded the information already available in the event as I thought that would be fine. Is there an easy way to get the internal id from here?
Makes these event types webhook compatible: