fix(MailQueueHandler): check enable_email toggle before sending queued emails#2563
Open
miaulalala wants to merge 2 commits intomasterfrom
Open
fix(MailQueueHandler): check enable_email toggle before sending queued emails#2563miaulalala wants to merge 2 commits intomasterfrom
miaulalala wants to merge 2 commits intomasterfrom
Conversation
…d emails sendEmails() was a raw consumer of the activity_mq queue with no knowledge of the admin enable_email toggle. Emails queued before the toggle was disabled would still be sent by the background job on its next run, because only the queuing paths (Consumer::receive and bulkReceive) checked the setting — the send path did not. Add an early-return guard at the top of sendEmails() that mirrors the check already present in UserSettings::getUserSetting() and filterUsersBySetting(). The queue is intentionally left intact so that if the admin re-enables emails, any pending notifications can still be delivered. Signed-off-by: Anna Larch <anna@nextcloud.com> AI-Assisted-By: claude-sonnet-4-6
… toggle Add testSendEmailsSkipsWhenAdminEmailDisabled: when enable_email=no, sendEmails must return 0, never call mailer->send(), and leave all queue entries intact. Signed-off-by: Anna Larch <anna@nextcloud.com> AI-Assisted-By: claude-sonnet-4-6
Collaborator
Author
|
/backport to stable33 |
Activity
 |
||||||||||||||||||||||||||||
| Project |
Activity
|
| Branch Review |
fix/mail-queue-handler-email-toggle
|
| Run status |
|
| Run duration | 01m 59s |
| Commit |
|
| Committer | Anna |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
1
|
 |
0
|
 |
9
|
| View all changes introduced in this branch ↗︎ | |
Collaborator
Author
|
/backport to stable32 |
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
1 participant
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
The admin Enable notification emails toggle was only checked when queuing new emails (
Consumer::receive()/bulkReceive()). TheMailQueueHandler::sendEmails()background job that actually delivers emails had no knowledge of the toggle and would send all queued entries regardless.This means: if emails were already sitting in
oc_activity_mqwhen an admin disabled the toggle, they would still be sent on the next background job run.The fix adds an early-return guard at the top of
sendEmails(), consistent with the checks already inUserSettings::getUserSetting()andfilterUsersBySetting(). The queue is intentionally left intact — if the admin re-enables emails, pending notifications can still be delivered.Related to #2562 (which fixed the same missing check in
bulkReceive()).Test plan
composer test:unitpasses (327 tests)testSendEmailsSkipsWhenAdminEmailDisabledverifies: return value is 0, mailer is never called, queue entries survive🤖 Generated with Claude Code