[Vortex] Support for sending e-mail notifications as a batch

Review Request #562 — Created June 6, 2018 and submitted

k.woortman
Lunr
emailBatches
59cc4d0...
lunr

Support for sending e-mail notifications as a batch

Unit tests.

  • 0
  • 0
  • 14
  • 1
  • 15
Description From Last Updated
There are no open issues
pprkut
  1. 
      
  2. src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 1)
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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.

  3. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    The issue has been dropped. Show all issues

    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

  4. 
      
k.woortman
pprkut
  1. 
      
  2. src/Lunr/Vortex/Email/EmailBatchResponse.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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 ;)

  3. src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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.

  4. 
      
k.woortman
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    please don't :)

  3. Rest looks good! Now unit tests :)

k.woortman
k.woortman
pprkut
  1. 
      
  2. src/Lunr/Vortex/Email/Tests/EmailResponseTest.php (Diff revision 5)
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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.

  3. 
      
k.woortman
pprkut
  1. 
      
  2. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 6)
     
     
    The issue has been resolved. Show all issues

    'array', lower case pls

  3. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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.

  4. The issue has been resolved. Show all issues

    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.

  5. 
      
k.woortman
smillernl
  1. Ship It!
  2. 
      
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    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)

  3. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    preferably split this off into a separate method. Don't like too much logic directly in the constructor :/

  4. 
      
k.woortman
pprkut
  1. 
      
  2. src/Lunr/Vortex/Email/EmailDispatcher.php (Diff revision 8)
     
     
     
     
    The issue has been resolved. Show all issues

    putting this in a finally clause makes it a bit more clear IMHO

  3. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 8)
     
     
    The issue has been resolved. Show all issues

    try to be explicit in such cases, easier to read (i.e. === FALSE)

  4. The issue has been resolved. Show all issues

    that's a bit much for one line, please make it multiline

    the other one too

  5. 
      
k.woortman
k.woortman
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    Please make it extend the MultiDispatcher interface now

  3. 
      
k.woortman
k.woortman
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...