Improve e-mail recipient logic, switch to Django's e-mail support, and add unit tests

 






 

 


  Review Request #44
  —
  Created June 1, 2007 and submitted

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board SVN (deprecated)
 









 
 

  trunk
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Several improvements to the e-mail support here.

* Recipient logic is now improved. We always e-mail to the review request author and the target reviewers, but we also add to this in some cases:
  * Updates to a diff or review request go to every person who has been involved in a discussion on the review request.
  * A reply on a review goes to every person who has been involved in that review's discussion.

* We now use Django's e-mail support instead of our own, now that we can subclass their new EmailMessage object and inject the headers we need. This also gives us the next feature..
* Unit tests! We now unit test e-mail. We specifically check the subject and the To: list.

 








  


 
 

  
Ran the unit tests. They worked.

 







 









 





  








 









 






  

 








 














 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Mostly looks good.  Did you forget to include the fixture file?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Nope. It was just really big and ugly, so I left it out of the generated diff. It'll be committed.
      
 
      

      2. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/reviews/email.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I'd prefer to see this called "ThreadedEmailMessage" or somesuch.  "Spiffy" just doesn't give me any information about what it does.

Alternatively, explain it with a docstring.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yep. I couldn't come up with a name I liked, so I left it as-is, expecting that you'd come up with one :)
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/reviews/email.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    This could probably be slimmed down with a list comprehension, but not critical.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I was trying to figure out how to do that without ending up with a [[...], [...], ...].

If I did:

recipient_list += [harvest_people_from_review(reply) for reply in review.replies.all()]

I would get:

[["chipx86", "admin"], ["foo", "bar"]] and such.

I'd love to slim this down. Suggestions?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I think [recipient_list += harvest_people_from_review(reply) for reply in review.replies.all()] will work.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Found a solution, as we discussed in IRC. I'm quite happy by it, though it feels so backwards. Thanks Python.
      
 
      

      4. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/reviews/email.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Same for this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...