-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: refactor reminders #2223
feat: refactor reminders #2223
Conversation
…q/monica into 2018-12-22-fix-one-time-reminder
…q/monica into 2018-12-22-fix-one-time-reminder
…q/monica into 2018-12-22-fix-one-time-reminder
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.
SonarQube analysis found issues:
Bugs: 0
Vulnerabilities: 0
Code Smells: 18
Including the following issue(s) which could not be reported in line:
@asbiin ready for review 😀 |
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.
See my review here.
Some little change request, and questions.
Thanks @djaiss !
database/migrations/2019_01_05_152526_schedule_new_reminders.php
Outdated
Show resolved
Hide resolved
@@ -72,15 +74,19 @@ public function execute(array $data) : LifeEvent | |||
private function addYearlyReminder($data, $lifeEvent) | |||
{ | |||
if ($data['has_reminder']) { | |||
$array = [ | |||
$date = Carbon::parse($data['happened_at']); |
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.
You should use
$date = Carbon::parse($data['happened_at']); | |
$date = DateHelper::parseDate($data['happened_at']); |
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.
No because it's Y-m-d format and this helper needs H:i:s
as well. It expects DateTime and I have a Date.
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.
Nope, DateHelper::parseDateTime
parse a DateTime, but DateHelper::parseDate
needs a Date format, and render a Date format.
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.
ahhhh ok I get it.
This pull request has been automatically locked since there |
This PR completely refactors how reminders are managed.
Notification
object has been completely removed.reminders
reminder_outbox
reminder_sent
reminder_id
column anymoreinitial_date
.nature
field which can contain two values:reminder
andnotification
.notification
means that it's a reminder that needs to be sent a few days the actual reminder is sent - these correspond to the Reminder Rules set in the account.New processes
reminder
typereminder_sent
table and delete the Reminderoutbox object.send:reminders
command which takes care of everything.reminder_id
anymore. It shouldn't be special dates' responsibilities to know if they have a reminder associated with them. All reminders should be linked to contacts. Therefore, we got rid of thesetReminder
method in thespecialDate
class.delible
field in the reminder object.Other changes
Limitations