Skip to content
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

Todo reminder notification #73

Merged
merged 8 commits into from
Mar 19, 2016

Conversation

rommsen
Copy link
Contributor

@rommsen rommsen commented Mar 15, 2016

This is a follow up to #71 (PR to wrong branch).

Compared to the old pull request I added a distinct read model for todo reminders. This read model is used by the worker to get reminders that should be used for notification.

To run the worker with docker:
docker-compose run --rm php php ./scripts/send_reminder_notification.php

rommsen added 4 commits March 15, 2016 14:46
…gnee.

includes worker script, Command, Event and a new projection
The read model is used by the send_reminder_notification worker.
@rommsen rommsen changed the title Todo reminder notification [WIP] Todo reminder notification Mar 15, 2016
/**
* This script looks for todos with open reminders and reminds the assignees
*/
namespace {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have proophessor-do namespace, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to match the other scripts in the folder. Dont mind adding the namespace

@rommsen rommsen force-pushed the todo_reminder_notification branch from e7c42c9 to d193a42 Compare March 15, 2016 14:50
@rommsen
Copy link
Contributor Author

rommsen commented Mar 16, 2016

atm I can't install Zend\Mail because of zendframework/zend-mime#10. In the meantime is there anything else I can do with this PR or are you guys happy with it as it is?

@rommsen
Copy link
Contributor Author

rommsen commented Mar 17, 2016

Alright. I added Zend\Mail to send actual mails.
I tried to implement it as less intervening as possible, i.e. the default transport is in_memory. If a user wants to use smtp he can copy the smtp_connection.local.php.dist to smtp_connection.local.php and fill in the credentials (gmail is default). I have never used container-interop nor the zend container so I am not 100% percent sure whether this is correct or not.

@rommsen
Copy link
Contributor Author

rommsen commented Mar 17, 2016

Writing this I realized mail_connection.local.php.dist is a much better name

@codeliner
Copy link
Member

@rommsen PR looks great. I'll take a deeper look and test it at the weekend

return new self(
new \DateTimeImmutable($reminder, new \DateTimeZone('UTC')),
TodoReminderStatus::fromString($status)
);
}

/**
* @param string $reminder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\DateTimeImmutable

@codeliner codeliner changed the title [WIP] Todo reminder notification Todo reminder notification Mar 19, 2016
codeliner added a commit that referenced this pull request Mar 19, 2016
@codeliner codeliner merged commit 766bd52 into prooph:develop Mar 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants