|
|
|
Created:
16 years, 4 months ago by kaelog Modified:
16 years, 3 months ago Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
message by XMPP
Patch Set 1 #
Total comments: 13
Patch Set 2 : Adding the xmpp status in the front of account name - when enabled. #Patch Set 3 : Adding the xmpp status in the front of account name - when enabled. #
Total comments: 31
Total messages: 8
|
kaelog
|
16 years, 4 months ago (2009年09月07日 12:51:43 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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.
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.
[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
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?
FYI, see http://9.latest.codereview.appspot.com/126070 for a reincarnation of this patch.
