-
-
src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 1) I'd rather not have BCC emails. Having the endpoint in the TO: header is fine, we just want to avoid setting the same information over and over again in the code.
-
src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 1) Maybe we can extend this now? Not sure the UNKNOWN status is a relic from pre-PHPMailer days or if PHPMailer just doesn't have more info
[Vortex] Support for sending e-mail notifications as a batch
Review Request #562 — Created June 6, 2018 and submitted
Information | |
---|---|
k.woortman | |
Lunr | |
emailBatches | |
59cc4d0... | |
Reviewers | |
lunr | |
Support for sending e-mail notifications as a batch
Unit tests.
Change Summary:
Email notifications are not send as BCC anymore.
Testing Done: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+102 -20) |
-
-
src/Lunr/Vortex/Email/EmailBatchResponse.php (Diff revision 2) if you need a BatchResponse class then it should be the other way around. The main usage should always be on the plain Response class, and then use the BatchResponse class in there if you have to.
However, I don't see any complexity that warrants that. So try to not introduce it ;)
-
src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 2) There's not really a good reason to have this split into a separate method. It neither serves readability nor code reuse. so better just leave it inline.
Change Summary:
Removed EmailBatchResponse class and factor out unnecessary method
Testing Done: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||
Diff: |
Revision 3 (+60 -43) |
Change Summary:
Missing asterisk in file doc comment
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+59 -42) |
Change Summary:
Add unit tests for EmailResponses
Testing Done: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 5 (+148 -206) |
-
-
src/Lunr/Vortex/Email/Tests/EmailResponseTest.php (Diff revision 5) Please don't put actual tests in the main test class. Keep them separate. Typically we put all tests for one method in a separate class.
Change Summary:
Seperate EmailResponse tests in multiple classes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+218 -225) |
-
-
-
src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 6) The reason why we typically don't have setters on the response objects is to keep them from being modified after.
It's maybe not as pretty, but constructing an array in the dispatcher and then passing that array to the response class on instantiation would mostly have the same effect here. Logging can stay in the response class.
-
src/Lunr/Vortex/Email/Tests/EmailResponseSetStatusTest.php (Diff revision 6) Mocks like this we typically still create in the setUp method. We only really do them in-place if theey are used in corner cases or they can't be instantiated generically.
Change Summary:
Set response object after sending email notifications
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+222 -195) |
-
-
src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 7) I'm afraid, this doesn't work. You're not cloning the class, so the status won't be kept and the result class will only ever see the last result.
You need to store the actual result here (cloning the object would be a waste)
-
src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 7) preferably split this off into a separate method. Don't like too much logic directly in the constructor :/
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+249 -200) |
-
-
src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 8) putting this in a finally clause makes it a bit more clear IMHO
-
src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 8) try to be explicit in such cases, easier to read (i.e. === FALSE)
-
src/Lunr/Vortex/Email/Tests/EmailResponseTest.php (Diff revision 8) that's a bit much for one line, please make it multiline
the other one too
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+261 -200) |
Change Summary:
squash commits
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+261 -200) |
-
-
src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 10) Please make it extend the MultiDispatcher interface now
Change Summary:
EmailDispatcher implements PushNotificationMultiDispatcherInterface
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+263 -202) |