fix(MailQueueHandler): check enable_email toggle before sending queued emails#2563
Open
miaulalala wants to merge 1 commit into
Open
fix(MailQueueHandler): check enable_email toggle before sending queued emails#2563miaulalala wants to merge 1 commit into
miaulalala wants to merge 1 commit into
Conversation
Collaborator
Author
|
/backport to stable33 |
Activity
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Activity
|
| Branch Review |
fix/mail-queue-handler-email-toggle
|
| Run status |
|
| Run duration | 02m 38s |
| Commit |
|
| Committer | Anna |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
2
|
 |
0
|
 |
1
|
 |
0
|
 |
7
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
| Test | Artifacts | |
|---|---|---|
| Check activity listing in the sidebar > Has favorite activity |
Test Replay
Screenshots
|
|
| Test | Artifacts | |
|---|---|---|
| Check that user's settings survive a reload > Form survive a reload |
Test Replay
Screenshots
|
|
Collaborator
Author
|
/backport to stable32 |
come-nc
reviewed
May 12, 2026
| * @return int Number of users we sent an email to | ||
| */ | ||
| public function sendEmails(int $limit, int $sendTime, bool $forceSending = false, ?int $restrictEmails = null): int { | ||
| if ($this->config->getAppValue('activity', 'enable_email', 'yes') === 'no') { |
Contributor
There was a problem hiding this comment.
Might as well inject IAppConfig and use that instead of the deprecated getAppValue, that will avoid having to adapt both the class and the test later on.
miaulalala
force-pushed
the
fix/mail-queue-handler-email-toggle
branch
from
cb519bb to
e2703fe
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…d emails The admin Enable notification emails toggle was only checked when queuing new emails. The sendEmails() background job had no knowledge of it and would send all queued entries regardless. Adds an early-return guard using IAppConfig::getValueString() at the top of sendEmails(), consistent with the checks in UserSettings. The queue is left intact so pending notifications can be delivered if the admin re-enables emails. AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
force-pushed
the
fix/mail-queue-handler-email-toggle
branch
from
e2703fe to
bbd2758
Compare
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
2 participants
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