Feat: make some events webhook compatible

[janepie]

Makes these event types webhook compatible:

• NewMessageReceivedEvent
• MessageSentEvent
• MessageFlaggedEvent
• MessageDeletedEvent

[welcome]

Thanks for opening your first pull request in this repository! ✌️

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds webhook serialization support to message events. MessageDeletedEvent, MessageFlaggedEvent, and MessageSentEvent each implement IWebhookCompatibleEvent with a getWebhookSerializable() method exposing relevant event properties. NewMessageReceivedEvent undergoes a broader structural change: its constructor now accepts both a URI string and a Message entity (previously only the URI), and its webhook serialization exports both fields. The NewMessagesNotifier listener is updated to dispatch the event with both parameters.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main objective of the changeset: making specific event classes webhook compatible by implementing IWebhookCompatibleEvent.
Description check	✅ Passed	The description directly relates to the changeset by listing the four event classes being made webhook compatible, which matches the actual file modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

lib/Events/MessageSentEvent.php (1)

40-43: ⚡ Quick win

Stabilize the webhook contract with an explicit payload schema.

Serializing LocalMessage directly 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 win

Avoid emitting raw Message entities 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

📥 Commits

Reviewing files that changed from the base of the PR and between 114629c and cfb6a98.

📒 Files selected for processing (5)

• lib/Events/MessageDeletedEvent.php
• lib/Events/MessageFlaggedEvent.php
• lib/Events/MessageSentEvent.php
• lib/Events/NewMessageReceivedEvent.php
• lib/Listener/NewMessagesNotifier.php

[kesselb]

@janepie is the target 34?

[janepie]

@janepie is the target 34?

Would be nice but I think I can live with 35, let me doublecheck

[janepie]

Checked, 35 is fine!

[kesselb]

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

public function get(int $id): DataResponse {

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).

[janepie]

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?

[kesselb]

Sorry @janepie for the slow response 🙈

I'd pushed a slight different version of the change.

[janepie]

All good, I also left it lying around a bit too long 😅

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[janepie]

What kind of tests would you want to have for this?

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

lib/Events/MessageSentEvent.php:45

• Webhook field naming is inconsistent with the other events added/updated in this PR: sendAt vs sentAt (used by NewMessageReceivedEvent) and inReplyTo vs inReplyToMessageId. For webhook consumers, consistent key names across event types reduces special-casing; consider aligning to a shared naming scheme (e.g., sentAt and inReplyToMessageId).

	public function getWebhookSerializable(): array {
		return [
			'messageId' => $this->localMessage->getId(),
			'accountId' => $this->localMessage->getAccountId(),
			'sendAt' => $this->localMessage->getSendAt(),
			'subject' => $this->localMessage->getSubject(),
			'inReplyTo' => $this->localMessage->getInReplyToMessageId(),
			'failed' => $this->localMessage->isFailed()
		];
	}

lib/Service/MailManager.php:443

• This introduces a silent early return that skips dispatching MessageFlaggedEvent (and therefore skips downstream behavior) when the message isn’t found in the cache. Even if this is expected to be rare, it will be very hard to diagnose in production without a log entry. Consider logging a warning (including mailbox id/name + uid) before returning so missing-cache situations can be traced.

		// Looking the message up by uid is a shortcut to avoid changing this method's
		// signature, which the JMAP PR does anyway.
		$messages = $this->dbMessageMapper->findByUids($mb, [$uid]);
		if (count($messages) < 1) {
			// The message should be in the database cache, otherwise the client wouldn't
			// know about the uid. Skip the event rather than fail the whole flag operation.
			return;
		}

tests/Unit/Listener/MessageCacheUpdaterListenerTest.php:49

• These tests now call mapper->update($message) with a Message instance that only has a UID set. In production, update() on a DB entity typically expects the primary key (id) to be set as well; keeping the test entity closer to a real persisted message (e.g., set an id) will make the unit tests more robust against mapper validation changes and better reflect the production contract.

		$message = new Message();
		$message->setUid(123);

tests/Unit/Listener/MessageCacheUpdaterListenerTest.php:60

• These tests now call mapper->update($message) with a Message instance that only has a UID set. In production, update() on a DB entity typically expects the primary key (id) to be set as well; keeping the test entity closer to a real persisted message (e.g., set an id) will make the unit tests more robust against mapper validation changes and better reflect the production contract.

			->method('update')
			->with($message);

[kesselb]

I've modified and extended the PR a bit to address some inconsistencies found during review. For example, the messageId in MessageDeletedEvent is actually the message's UID. I hope it's a bit cleaner now, but it needs fresh review. @ChristophWurst @DerDreschner

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

lib/Events/MessageFlaggedEvent.php:27

• The constructor signature changes from (… , int $uid, …) to (…, Message $message, …), which is a breaking change for any existing event dispatchers/listeners. To preserve compatibility, consider adding a deprecated alternate constructor/factory (e.g., fromUid(Account $account, Mailbox $mailbox, int $uid, …)) that creates a lightweight Message with just the UID set, while keeping the new Message-based constructor as the preferred path.

class MessageFlaggedEvent extends Event implements IWebhookCompatibleEvent {
	public function __construct(
		private Account $account,
		private Mailbox $mailbox,
		private Message $message,
		private string $flag,
		private bool $set,
	) {
		parent::__construct();
	}

lib/Events/NewMessageReceivedEvent.php:25

• The event previously appears to have been constructible with only a URI; it now requires Account, Mailbox, and Message. If any external/other internal code constructs this event using the old signature, this is a breaking change. Consider supporting the previous constructor shape via an additional named constructor (deprecated) or by making the extra parameters optional when webhook serialization isn’t needed.

class NewMessageReceivedEvent extends Event implements IWebhookCompatibleEvent {
	public function __construct(
		private Account $account,
		private Mailbox $mailbox,
		private Message $message,
		private string $uri,
	) {
		parent::__construct();
	}

lib/Events/MessageDeletedEvent.php:24

• This changes the public API from messageId/getMessageId() to uid/getUid(). If this event is consumed outside this PR’s updated call sites, it’s a breaking change. Consider keeping a deprecated getMessageId(): int method that forwards to getUid(), and (optionally) accepting the old constructor parameter name via a deprecated named constructor/factory to ease upgrade.

class MessageDeletedEvent extends Event implements IWebhookCompatibleEvent {
	public function __construct(
		private Account $account,
		private Mailbox $mailbox,
		private int $uid,
	) {
		parent::__construct();
	}

lib/Events/MessageDeletedEvent.php:36

• This changes the public API from messageId/getMessageId() to uid/getUid(). If this event is consumed outside this PR’s updated call sites, it’s a breaking change. Consider keeping a deprecated getMessageId(): int method that forwards to getUid(), and (optionally) accepting the old constructor parameter name via a deprecated named constructor/factory to ease upgrade.

	public function getUid(): int {
		return $this->uid;
	}

lib/Service/MailManager.php:443

• This silently skips dispatching MessageFlaggedEvent when the message isn’t in the DB cache. That can make webhook/flag-related behavior hard to diagnose in production. Consider logging at least a warning/debug entry including mailbox + UID so operators can understand why expected webhook/cache updates didn’t occur.

		// Looking the message up by uid is a shortcut to avoid changing this method's
		// signature, which the JMAP PR does anyway.
		$messages = $this->dbMessageMapper->findByUids($mb, [$uid]);
		if (count($messages) < 1) {
			// The message should be in the database cache, otherwise the client wouldn't
			// know about the uid. Skip the event rather than fail the whole flag operation.
			return;
		}

lib/Events/MessageSentEvent.php:44

• The webhook payload sources accountId from LocalMessage even though the event already carries an Account. For consistency with the other webhook events (which use $this->account->getId()), and to avoid cases where LocalMessage::getAccountId() might be unset on non-persisted instances, consider using the Account object as the source of truth. Also, the timestamp key sendAt differs from sentAt used elsewhere; if these represent the same concept, standardizing the key would reduce consumer confusion.

	public function getWebhookSerializable(): array {
		// No local message id: the row is deleted right after a successful send
		return [
			'accountId' => $this->localMessage->getAccountId(),
			'inReplyToRfcMessageId' => $this->localMessage->getInReplyToMessageId(),
			'sendAt' => $this->localMessage->getSendAt(),
			'subject' => $this->localMessage->getSubject(),
		];
	}