[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
pprkut
  1. 
      
  2. 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.

  3. 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

  4. 
      
k.woortman
pprkut
  1. 
      
  2. 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 ;)

  3. 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.

  4. 
      
k.woortman
pprkut
  1. 
      
  2. 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)
     
     
     
     
     
     
     
     
     

    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)
     
     

    'array', lower case pls

  3. 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.

  4. 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. 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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     

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

  3. src/Lunr/Vortex/Email/EmailResponse.php (Diff revision 8)
     
     

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

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

    the other one too

  5. 
      
k.woortman
k.woortman
pprkut
  1. 
      
  2. 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...