Message by XMPP instead of email when available

message by XMPP

[parren, 2009-09-07 14:43:40]

Just a typo.

http://codereview.appspot.com/115067/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/115067/diff/1/3#newcode2376
Line 2376: logging.info('%s did not configure sned by xmpp'%reviewer)
sned -> send

[Andi Albrecht, 2009-09-09 14:27:28]

If we enable this type of notification, I think we need to handle incoming XMPP
messages too since the messages pops up in some chat window and just in case a
user replies to the message he/she should get a response from the server that
the reply wasn't handled... :(

I'm not sure if we shouldn't send a different message when sending over XMPP.
The whole message, including quoted previous messages or patches and comments
with context lines, seems to be a bit too much for a chat window. For e-mail and
the web frontend that is not so much a problem than with IM where the message
windows are usually pretty small. An e-mail is sent to the user as well. Maybe
it would be enough to inform the user via XMPP about changes. But in general I'm
hesitating if there's much benefit in adding this kind of notification.

However, here are some comments :)

http://codereview.appspot.com/115067/diff/1/4
File codereview/models.py (right):

http://codereview.appspot.com/115067/diff/1/4#newcode515
Line 515: send_by_xmpp = db.BooleanProperty(default=False)
No need to add a newline here.

http://codereview.appspot.com/115067/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/115067/diff/1/3#newcode341
Line 341: send_by_xmpp = forms.BooleanField(required=False)
There are leading whitespaces in the line above and below.

http://codereview.appspot.com/115067/diff/1/3#newcode2358
Line 2358: 
Whitespaces in empty line.

http://codereview.appspot.com/115067/diff/1/3#newcode2359
Line 2359: for reviewer in reviewers:
Why not include the CC's here? Maybe they're interested too :)

http://codereview.appspot.com/115067/diff/1/3#newcode2361
Line 2361: account = models.Account.get_account_for_email(reviewer)
Please remove any leading and trailing whitespaces - like the trailing
whitespaces in the line above :)

I wonder if it wouldn't be useful to refactor some code. The accounts are
already fetched when reviewer_nicknames is created (line 2339 via
library.nickname).

http://codereview.appspot.com/115067/diff/1/3#newcode2362
Line 2362: if account != None :
Whitespaces around ":" (here and in the following lines)

http://codereview.appspot.com/115067/diff/1/3#newcode2364
Line 2364: if xmpp.get_presence(reviewer) :
I think we should do this (or the mail sending stuff in general) in a taskqueue.

http://codereview.appspot.com/115067/diff/1/3#newcode2369
Line 2369: reviewers.remove(reviewer)
Why do you remove reviewer? The email is still send to the reviewer since he's
listed in the "to" variable. I'm pretty unsure if it's a good idea not to send
an email if the users has selected XMPP - think of a user who want's to be
informed about changes by XMPP immediately but want's to receive the mails too.

http://codereview.appspot.com/115067/diff/1/3#newcode2645
Line 2645: 'send_by_xmpp':send_by_xmpp
Missing whitespace after ":" and missing comma at end of the line.

http://codereview.appspot.com/115067/diff/1/3#newcode2654
Line 2654: account.send_by_xmpp = form.cleaned_data.get('send_by_xmpp')
An invitation is needed if this is set to True for the first time. Otherwise the
XMPP won't be send to the user since get_presence() always returns False. I
think this procedure should be mentioned on the settings page as well.
Maybe a button on the settings page to re-send the invitation is useful too.

[kaelog, 2009-09-13 16:52:08]

http://codereview.appspot.com/115067/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/115067/diff/1/3#newcode2364
Line 2364: if xmpp.get_presence(reviewer) :
It's true, I think the next step for XMPP, could be, if the user is not
connected, retry later.

On 2009/09/09 14:27:28, Andi Albrecht wrote:
> I think we should do this (or the mail sending stuff in general) in a
taskqueue.

http://codereview.appspot.com/115067/diff/1/3#newcode2369
Line 2369: reviewers.remove(reviewer)
From my point of view, the main goal to add XMPP notification is to avoid to
receive emails. I don't like very much to receive emails from application just
for notification. Every information in the email can be found in the web GUI.

On 2009/09/09 14:27:28, Andi Albrecht wrote:
> Why do you remove reviewer? The email is still send to the reviewer since he's
> listed in the "to" variable. I'm pretty unsure if it's a good idea not to send
> an email if the users has selected XMPP - think of a user who want's to be
> informed about changes by XMPP immediately but want's to receive the mails
too.

[Andi Albrecht, 2009-09-18 11:27:17]

[Adding Guido as CC]

I'm still not sure if the "send_by_xmpp" flag is really exclusive or if it's a
valid use case that a user wants both, a notification via XMPP and the review
mail. Any opinions on this?

Property changes in '.': We really don't want to ignore app.yaml and main.py :)

icons_ns6.png: Under which license is this image available?

http://codereview.appspot.com/115067/diff/2017/1018
File app.yaml (right):

http://codereview.appspot.com/115067/diff/2017/1018#newcode28
Line 28: 
There are whitespaces here...

http://codereview.appspot.com/115067/diff/2017/1018#newcode31
Line 31: 
...and here :)

http://codereview.appspot.com/115067/diff/2017/1024
File codereview/library.py (right):

http://codereview.appspot.com/115067/diff/2017/1024#newcode85
Line 85: ret = ('<a href="/user/%(key)s" onMouseOver="M_showUserInfoPopup(this)"
class="%(class)s">%(key)s</a>'
Line too long.

http://codereview.appspot.com/115067/diff/2017/1024#newcode100
Line 100: 
Can you please remove the whitespaces in empty lines and at eventually trailing
whitespaces at the end of lines.

http://codereview.appspot.com/115067/diff/2017/1024#newcode102
Line 102: if xmpp.get_presence(email) :
hm, I think we either shouldn't display that status icon in front of the
nickname or cache it for a while. The presence will be checked each time the
nickname is displayed, which easily hits available quote limits - and it's
redundant in a certain manner. IMO it's ok to leave this show_user filter as is
and display the status on the issue per user page and maybe in the sidebar on
the issue overview page.

http://codereview.appspot.com/115067/diff/2017/1024#newcode106
Line 106: ret = '<img src="/static/cleardot.gif" alt="%s" class="xmppstatus
xmpp%s"/>%s' % (status,status,ret)
Line too long

http://codereview.appspot.com/115067/diff/2017/1023
File codereview/models.py (right):

http://codereview.appspot.com/115067/diff/2017/1023#newcode513
Line 513: uploadpy_hint = db.BooleanProperty(default=True
Did you test this? This is actually a syntax error :(

http://codereview.appspot.com/115067/diff/2017/1022
File codereview/views.py (right):

http://codereview.appspot.com/115067/diff/2017/1022#newcode339
Line 339: max_value=engine.MAX_COLUMN_WIDTH)
Trailing whitespaces again. I don't comment further leading and trailing
whitespaces or lines that are > 79 chars. Please make sure, that empty lines are
really empty, no trailing whitespaces are at the end of lines and that lines are
not longer than 79 chars.

http://codereview.appspot.com/115067/diff/2017/1022#newcode340
Line 340: send_by_xmpp = forms.BooleanField(required=False, label='Send by
XMPP', help_text='Your status will appear.')
Line too long.

http://codereview.appspot.com/115067/diff/2017/1022#newcode1437
Line 1437: """ Send to XMPP invitation to the logged user """
Duplicate docstring.

http://codereview.appspot.com/115067/diff/2017/1022#newcode1439
Line 1439: return HttpResponseRedirect('/settings')
I'd prefer a confirmation message on the settings page that the invitation has
been sent successfully.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2345
Line 2345: for rev_temp in reviewers)
Check identation, "for" should start below "library" and not the parenthesis.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2363
Line 2363: context.update({'subject':issue.subject})
Missing whitespace after ":"

http://codereview.appspot.com/115067/diff/2017/1022#newcode2364
Line 2364: xmpp_body =
django.template.loader.render_to_string(template.replace('mail','xmpp'),
context)
This won't work! You'll have to replace "mails" (trailing "s"), not "mail". BTW,
I'd prefer a different approach. Why not directly choose a XMPP template,
instead of this generic approach.

We only need to render the template, when there's at least one recipient.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2365
Line 2365: for reviewer in reviewers + cc:
What about the issue owner?

http://codereview.appspot.com/115067/diff/2017/1022#newcode2370
Line 2370: if xmpp.get_presence(reviewer):
Do we need to check the presence before sending an XMPP message? At least GMail
accepts messages even when offline, but I don't know if that's a GMail specific
thing.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2375
Line 2375: cc.remove(reviewer)
This will raise a ValueError for one of both lists. Usually reviewer is only in
on of both lists.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2384
Line 2384: logging.info('No account linked to %s'%reviewer)
That's quite a lot of logging :) I think we can simplify that a bit.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2386
Line 2386: for rev_temp in reviewers)
IMO there's no need to build review_nicknames again. It's ok to list reviewers
who received an XMPP message too.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2653
Line 2653: logging.debug('Form %s'%form.initial)
some debugging code left behind?

http://codereview.appspot.com/115067/diff/2017/1022#newcode2661
Line 2661: account.send_by_xmpp = form.cleaned_data.get('send_by_xmpp')
Shouldn't it be cleaned_data.get('send_by_xmpp', False). AFAIK if the checkbox
isn't activated 'send_by_xmpp' isn't in request.POST and cleaned_data.get()
would return None.

http://codereview.appspot.com/115067/diff/2017/1022#newcode2662
Line 2662: if account.send_by_xmpp and account.send_xmpp_invitation :
Whitespace before ":"

http://codereview.appspot.com/115067/diff/2017/1022#newcode2664
Line 2664: account.send_xmpp_invitation = False
I'd prefer to remove "send_xmpp_invitation" again. The information is already
present in "send_by_xmpp": If it would default to None (interpreted as False by
the send message function) it would mean, that the initial note on the settings
page should be displayed. If "send_by_xmpp" is False or True the settings page
should show the "Send reinvite" link.

http://codereview.appspot.com/115067/diff/2017/1030
File index.yaml (right):

http://codereview.appspot.com/115067/diff/2017/1030#newcode57
Line 57: - kind: Issue
Where doesn this index comes from? Where do we need it?

http://codereview.appspot.com/115067/diff/2017/1029
File templates/settings.html (right):

http://codereview.appspot.com/115067/diff/2017/1029#newcode20
Line 20: <a href="/invite">Resend the XMPP invitation</a>
I'd prefer a button and a POST request to /invite here. In addition the invite()
function should be decorated with the @post_required decorator.

http://codereview.appspot.com/115067/diff/2017/1027
File templates/xmpp/comment.txt (right):

http://codereview.appspot.com/115067/diff/2017/1027#newcode1
Line 1: {%autoescape off%}{{my_nickname}} has commented the issue : {{subject}}.
Whitespace before ":"

http://codereview.appspot.com/115067/diff/2017/1026
File templates/xmpp/review.txt (right):

http://codereview.appspot.com/115067/diff/2017/1026#newcode1
Line 1: {%autoescape off%}{{my_nickname}} has created an issue : {{subject}}.
Whitespace before ":"

http://codereview.appspot.com/115067/diff/2017/1031
File xmpp.py (right):

http://codereview.appspot.com/115067/diff/2017/1031#newcode8
Line 8: message.reply("Your are talking to a application....")
a -> an

[GvR, 2009-09-18 17:15:42]

My main wonderment is...  Why does it have to be so complex?  Why touch so many
files?

http://codereview.appspot.com/115067/diff/2017/1022
File codereview/views.py (right):

http://codereview.appspot.com/115067/diff/2017/1022#newcode2651
Line 2651: 'send_by_xmpp': send_by_xmpp
Please add trailing comma.

http://codereview.appspot.com/115067/diff/2017/1030
File index.yaml (right):

http://codereview.appspot.com/115067/diff/2017/1030#newcode57
Line 57: - kind: Issue
On 2009/09/18 11:27:17, Andi Albrecht wrote:
> Where doesn this index comes from? Where do we need it?

Presumably from the list of Closed issues in /mine ?

http://codereview.appspot.com/115067/diff/2017/1028
File templates/issue_base.html (right):

http://codereview.appspot.com/115067/diff/2017/1028#newcode87
Line 87: <div><b>Reviewers: </b><br/>
Why do you need a space here?

[GvR, 2009-10-01 18:32:15]

FYI, see http://9.latest.codereview.appspot.com/126070 for a reincarnation of
this patch.