-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip disabled users and do not break the cronjob on the first unsent email #842
Conversation
VicDeo
commented
Mar 9, 2020
•
edited
Loading
edited
- we don't need to fix DI since Allow background jobs to be service names for DI core#21281
- Do not try to send notification if the user is disabled
- Keep on sending emails after the exception
Codecov Report
@@ Coverage Diff @@
## master #842 +/- ##
============================================
- Coverage 84.38% 84.37% -0.01%
+ Complexity 507 504 -3
============================================
Files 47 47
Lines 1896 1895 -1
============================================
- Hits 1600 1599 -1
Misses 296 296
Continue to review full report at Codecov.
|
@@ -144,10 +142,14 @@ protected function runStep($limit, $sendTime) { | |||
$this->mqHandler->sendEmailToUser($uid, $user['email'], $language, $timezone, $sendTime); | |||
$sentMailForUsers[] = $uid; | |||
} catch (\Exception $e) { | |||
// Run cleanup before we die | |||
$this->mqHandler->deleteSentItems($sentMailForUsers, $sendTime); | |||
// Throw the exception again - which gets logged by core and the job is handled appropriately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the cleanup not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't rethrow exception any more which means that we always reach the line 155 now
https://github.com/owncloud/activity/pull/842/files#diff-35fc1857b1cf16ed158a97c77089f80cL155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, didn't see that,
👍
One question. Rest looking fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Coverage is miscalculating @VicDeo Can I merge now and override? |