Merge lp:~ampelbein/ubuntu-dev-tools/fix-850360 into lp:~ubuntu-dev/ubuntu-dev-tools/trunk

[Andreas Moog]

[Stefano Rivera (stefanor)]

I'd prefer .smtp_code to [0]

Can you wrap the long line?

Should we maybe dump the e-mail in . (or /tmp/) and print the filename if there's any error in sending it?

[lp:~ampelbein/ubuntu-dev-tools/fix-850360]

1246. By Andreas Moog on 2011-11-28

Wrap long line

[Andreas Moog (ampelbein)]

1+2 done.

I was thinking about saving the mail in /tmp/requestsync-$PACKAGE before sending and delete the file after succesfully sending the mail so that the user has it saved in case he wants to send manually later.

[lp:~ampelbein/ubuntu-dev-tools/fix-850360]

1247. By Andreas Moog on 2011-11-28

Save a temporary file in case sending fails

[Stefano Rivera (stefanor)]

* print -> Logger.normal, please
* spaces after commas, please

[lp:~ampelbein/ubuntu-dev-tools/fix-850360]

1248. By Andreas Moog on 2011-11-29

Use Logger.normal

1249. By Andreas Moog on 2011-11-29

Merge with trunk

[Andreas Moog (ampelbein)]

Ok, all done. Should work now.

[Stefano Rivera (stefanor)]

Traceback (most recent call last):
  File "./requestsync", line 348, in <module>
    main()
  File "./requestsync", line 344, in main
    mailserver_user, mailserver_pass)
  File "/home/stefanor/bzr/ubuntu-dev-tools/fix-850360/ubuntutools/requestsync/mail.py", line 166, in mail_bug
    f=open("/tmp/requestsync-" + srcpkg,"w")
TypeError: cannot concatenate 'str' and 'NoneType' objects

(Also, space after that comma, please :) )

Oh, and all 400-series errors are temporary, I suggest: 400 <= s.smtp_code < 500:

review: Needs Fixing

[Andreas Moog (ampelbein)]

I don't understand that traceback to be honest. Shouldn't srcpkg never be None? What command did you use to invoke requestsync? Or maybe i understand the error wrong.

Regarding the error codes, you are right, but 421 should be the only response that makes sense to retry at that point. It would perhaps make sense to implement another check after the s.sendmail() command to give a retry option in case of greylisting (reply code 450 in most cases).

Other temporary error aren't worthwhile to retry within a reasonable timeframe because they most often depend on admin interaction. (451/452).

[lp:~ampelbein/ubuntu-dev-tools/fix-850360]

1250. By Andreas Moog on 2011-12-02

Merge trunk

1251. By Andreas Moog on 2011-12-02

Space after comma

[Stefano Rivera (stefanor)]

srcpkg is None if it'll be a new package in Ubuntu.

[lp:~ampelbein/ubuntu-dev-tools/fix-850360]

1252. By Andreas Moog on 2011-12-03

Merge trunk

1253. By Andreas Moog on 2011-12-03

Use bugtitle as temporary filename

1254. By Andreas Moog on 2011-12-03

Also allow retry if recipient got rejected

1255. By Andreas Moog on 2011-12-03

Add a exception for disconnected server

1256. By Andreas Moog on 2011-12-03

Use regex to filter unwanted characters in filename

[Andreas Moog (ampelbein)]

Thank you Stefano for all your help and patience with me! Merge proposal updated!

[Stefano Rivera (stefanor)]

Made a few minor modifications, and merged.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2011-12-02 22:06:35 +0000
+++ debian/changelog	2011-12-03 17:15:28 +0000
@@ -27,6 +27,7 @@
   * sponsor-patch: Check permission to unsubscribe sponsors-team (LP: #896884)
   * grep-merges: We already require a UTF-8 enabled terminal, so encode 
     package and uploader name in UTF-8 (LP: #694388)
+  * requestsync: Give user option to retry in case of temporary error (LP: #850360)
 
  -- Andreas Moog <[e-mail]>  Wed, 30 Nov 2011 21:04:39 +0100
 

=== modified file 'ubuntutools/requestsync/mail.py'
--- ubuntutools/requestsync/mail.py	2011-11-13 20:50:34 +0000
+++ ubuntutools/requestsync/mail.py	2011-12-03 17:15:28 +0000
@@ -21,6 +21,7 @@
 #   of the GNU General Public License license.
 
 import os
+import re
 import sys
 import smtplib
 import socket
@@ -162,14 +163,30 @@
     print 'The final report is:\n%s' % mail
     confirmation_prompt()
 
+    # save mail in temporary file
+    f=open("/tmp/requestsync-" + re.sub("[^a-zA-Z0-9_\-]","",bugtitle.replace(" ","_")), "w")
+    f.write(mail)
+    f.close()
+
+    Logger.normal('The e-mail has been saved in %s and will be deleted '
+                  'after succesful transmission', f.name)
+
     # connect to the server
-    try:
-        Logger.info('Connecting to %s:%s ...', mailserver_host, mailserver_port)
-        s = smtplib.SMTP(mailserver_host, mailserver_port)
-    except socket.error, s:
-        Logger.error('Could not connect to %s:%s: %s (%i)',
-                     mailserver_host, mailserver_port, s[1], s[0])
-        return
+    while True:
+        try:
+            Logger.info('Connecting to %s:%s ...', mailserver_host, mailserver_port)
+            s = smtplib.SMTP(mailserver_host, mailserver_port)
+            break
+        except socket.error, s:
+            Logger.error('Could not connect to %s:%s: %s (%i)',
+                         mailserver_host, mailserver_port, s[1], s[0])
+            return
+        except smtplib.SMTPConnectError, s:
+            Logger.error('Could not connect to %s:%s: %s (%i)',
+                         mailserver_host, mailserver_port, s[1], s[0])
+            if s.smtp_code == 421:
+                confirmation_prompt(message='This is a temporary error, press '
+                          '[Enter] to retry. Press [Ctrl-C] to abort now.')
 
     if mailserver_user and mailserver_pass:
         try:
@@ -184,6 +201,21 @@
             s.quit()
             return
 
-    s.sendmail(myemailaddr, to, mail.encode('utf-8'))
-    s.quit()
-    Logger.normal('Sync request mailed.')
+    while True:
+        try:
+            s.sendmail(myemailaddr, to, mail.encode('utf-8'))
+            s.quit()
+            os.remove(f.name)
+            Logger.normal('Sync request mailed.')
+            break
+        except smtplib.SMTPRecipientsRefused, smtperror:
+            smtp_code, smtp_message = smtperror.recipients[to]
+            Logger.error('Error while sending: %i, %s', smtp_code, smtp_message)
+            if smtp_code == 450:
+                confirmation_prompt(message='This is a temporary error, press '
+                          '[Enter] to retry. Press [Ctrl-C] to abort now.')
+            else:
+                return
+        except smtplib.SMTPServerDisconnected:
+            Logger.error('Server disconnected while sending the mail.')
+            return