Email sending in a very primitive way is implemented.

Base rev: f3e0031ec3cc

First patch rev: 953c714b8d2e

[christophe.taton, 2009-10-03 21:00:27]

http://codereview.appspot.com/124105/diff/1/4
File robot/Emaily/src/com/google/wave/extensions/emaily/email/EmailSender.java
(right):

http://codereview.appspot.com/124105/diff/1/4#newcode48
Line 48: throw new RuntimeException("Cannot send email", e);
I'm wondering what should be the good behavior here.
The user should at least get some feedback that the send failed. Maybe a message
appended to the wave? This specific error-handling should not be done here
anyway, but I don't know if throwing a RuntimeException will help process this
error correctly later.
I'm just raising a question here, we may fix this later.

http://codereview.appspot.com/124105/diff/1/5
File
robot/Emaily/src/com/google/wave/extensions/emaily/robot/EmailyRobotServlet.java
(right):

http://codereview.appspot.com/124105/diff/1/5#newcode48
Line 48: * immediately.
Should this check that the blip just submitted contains the email gadget?
Also, it looks like that if we append a new blip to this wave, this will
currently send a new email with the content of the root blip (not the blip
appended).

http://codereview.appspot.com/124105/diff/1/5#newcode54
Line 54: // Find Email Subject
Why not use wavelet.getTitle()?

http://codereview.appspot.com/124105/diff/1/5#newcode71
Line 71: String senderEmail =
emailAddressUtil.emailAddressToWaveParticipantId(event.getBlip()
Nitpicking: line too long.

[dLux.hu, 2009-10-03 23:12:56]

Thanks for the review!

Please take another look at it!

Balázs

http://codereview.appspot.com/124105/diff/1/4
File robot/Emaily/src/com/google/wave/extensions/emaily/email/EmailSender.java
(right):

http://codereview.appspot.com/124105/diff/1/4#newcode48
Line 48: throw new RuntimeException("Cannot send email", e);
On 2009/10/03 21:00:28, christophe.taton wrote:
> I'm wondering what should be the good behavior here.
> The user should at least get some feedback that the send failed. Maybe a
message
> appended to the wave? This specific error-handling should not be done here
> anyway, but I don't know if throwing a RuntimeException will help process this
> error correctly later.
> I'm just raising a question here, we may fix this later.

Yes, error handling is necessary somehow. I'll add a todo for this. Appending
something to the text is a good idea, but it is not ready yet. I'll make a
gadget for this, and it needs to be figured out what is going to happen when the
email sending is failed. Retry? Offer to change email address?

http://codereview.appspot.com/124105/diff/1/5
File
robot/Emaily/src/com/google/wave/extensions/emaily/robot/EmailyRobotServlet.java
(right):

http://codereview.appspot.com/124105/diff/1/5#newcode48
Line 48: * immediately.
On 2009/10/03 21:00:28, christophe.taton wrote:
> Should this check that the blip just submitted contains the email gadget?

No, I changed my mind: I think it is not necessary. Everything should be email
if there is an email participant.

> Also, it looks like that if we append a new blip to this wave, this will
> currently send a new email with the content of the root blip (not the blip
> appended).

Whoops, yes, that's true. Fixing...

http://codereview.appspot.com/124105/diff/1/5#newcode54
Line 54: // Find Email Subject
On 2009/10/03 21:00:28, christophe.taton wrote:
> Why not use wavelet.getTitle()?

Well, I did not recognize it. But unfortunately it does not help too much, if
the email is sent from the root blip, I anyway have to calculate the content of
the message.

http://codereview.appspot.com/124105/diff/1/5#newcode71
Line 71: String senderEmail =
emailAddressUtil.emailAddressToWaveParticipantId(event.getBlip()
On 2009/10/03 21:00:28, christophe.taton wrote:
> Nitpicking: line too long.

Done.

[christophe.taton, 2009-10-04 07:42:27]

http://codereview.appspot.com/124105/diff/1005/15
File
robot/Emaily/src/com/google/wave/extensions/emaily/email/EmailAddressUtil.java
(right):

http://codereview.appspot.com/124105/diff/1005/15#newcode18
Line 18: * Returns an email address from a wave participant ID.
Actually, I find the naming still very confusing.
I cannot figure out what this does, even when I read the doc.
Also, the names of these two method look inverted: 

As I read the code, it seems
  waveParticipantIdToEmailAddress(participantId)
should be named
  emailAddressToWaveParticipantId(mailboxAddress)
with the following javadoc:

Parses and returns the wave participant ID encoded in an emaily mailbox address.

@param mailboxAddress An emaily mailbox address encoding a wave participant ID.
@return The wave participant ID encoded in the emaily mailbox address. Null if
the given email address is not a valid emaily mailbox address.

The other method should then be "inverted" as well.

What do you think?

[dLux.hu, 2009-10-04 10:17:10]

Hi Christophe,

Please take another look at it!

Balázs

http://codereview.appspot.com/124105/diff/1005/15
File
robot/Emaily/src/com/google/wave/extensions/emaily/email/EmailAddressUtil.java
(right):

http://codereview.appspot.com/124105/diff/1005/15#newcode18
Line 18: * Returns an email address from a wave participant ID.
On 2009/10/04 07:42:27, christophe.taton wrote:
> Actually, I find the naming still very confusing.
> I cannot figure out what this does, even when I read the doc.
> Also, the names of these two method look inverted: 
> 
> As I read the code, it seems
>   waveParticipantIdToEmailAddress(participantId)
> should be named
>   emailAddressToWaveParticipantId(mailboxAddress)
> with the following javadoc:
> 
> Parses and returns the wave participant ID encoded in an emaily mailbox
address.
> 
> @param mailboxAddress An emaily mailbox address encoding a wave participant
ID.
> @return The wave participant ID encoded in the emaily mailbox address. Null if
> the given email address is not a valid emaily mailbox address.
> 
> The other method should then be "inverted" as well.
> 
> What do you think?
> 
> 

Yes, actually these two methods do 4 different things, but they do the same in
pairs.

I'll rename the methods so that it is more easily understandable what are these
doing.

[christophe.taton, 2009-10-04 10:51:38]

LGTM.
Thanks for the renaming, that's better.