On Aug 14, 2009, at 06:35 PM, Данило Шеган wrote: > Данило Шеган has proposed merging lp:~danilo/launchpad/ > bug-410579 into lp:launchpad/devel. > > We should include more information, like what the request is for, and > how download link will expire in 1 week. We should also include > information on how you can re-request the download if you need to. Thanks for working on this Danilo. It looks like a very useful change that will help our users! > Subject should not mention your username, but instead a short > description of what the download is about. > > == Implementation details == > > The branch has quickly gotten very big (~900 lines), but it's mostly > because of textual changes to email templates. I hope it's not a > problem. > > * scripts/po_export_queue.py: > This is where the core change is: everything related to emailing is > moved into > ExportResult implementation, which generates appropriate emails. > > Two methods are of notice: > > * _getExportRequestOrigin(): figures out where export request was > most > likely made; > TranslationImportQueue keeps only a list of POTemplate and > POFile objects > and depending this method figures out the place where the > request was > "most likely" made (i.e. context where you could have requested > these > files, but then trying for the most relevant context as well). > > For instance, if we've got two pofiles on a single template, > request was > likely made on a potemplate object, so we return that (for > nicer URLs > and emails subjects though, if there's only one template in > a productseries/sourcepackage, we return them instead). > > * _getShortRequestName(request) takes the context returned by > previous method > and returns a nice short description in the form of 'Evolution > trunk' or > 'Ubuntu Karmic gimp-2.0 - Spanish translation' suitable for use > as email > subjects > > These are not unit tested, though they could be, simply because > they are > sufficiently well exercised by existing doctests, and because if > these start > failing, it'll be easy to notice (so, I'd rather not increase CPU > cycles > we spend on tests). They are mostly for 'nicer UI output', so > their correctness > is not critical to the operation. Do you really think it would take up that much more cycles to unit test these two methods? I'm generally not in favor of these kinds of indirect test when they can be reasonably avoided because that makes it much more difficult to change the methods later. You never really quite know what the effect will be, so you end up having to run the full test suite "just in case". With a unit test, you at least know you need to run that test if you change the method, and can presume that if those unit tests pass, everything else implicitly testing them will too. > > Other changes in this file are mostly for using emailtemplates > instead of hardcoding text. > > * emailtemplates/poexport-*.txt: > All the email templates have been moved out of the code and into > separate > template files. Some 75 lines of diff are basically > > * doc/poexport-request.txt, > doc/poexport-request-productseries.txt, > doc/poexport-queue.txt: > There are a lot of changes to tests that test how emails are > generated: > basically, half of the branch is there (440 lines). The changes > there > include introducing a shared helper print_mail_subject_and_body > method, > print subject along with body of the email, and changes to test new > email contents and subjects. Some minor changes to how > ExportResult is > constructed were necessary as well to conform with the new API > (i.e. pass > person, requested exports and logger). Do you know about lp.testing.mail_helpers.print_emails()? I generally use that plus ellipses to cut down on the output. Is there a reason why you'd want print_mail_subject_and_body instead? At the very least, any helpers you add should be added to lib/lp/testing/mail_helpers.py > To not increase size of the diff, some bits of the test are left > indented with 2 spaces. I'd be happy to fix before submitting. Sure, thanks! rs=me on those additional cleanups. > > * interfaces/translationimportqueue.py > Drive-by fix for #406799 (s/imported imported/imported/). Thanks! > > == Tests == > > bin/test -vvt poexport-request -t poexport-queue.txt > > == Demo and Q/A == > > Doctests above are the best way to demo this, because this involves > sending email around. > > QA on staging will consist of: > > 1. Try to download different things across > translations.staging.launchpad.net: > * all productseries translations > * single potemplate translations on productseries > * few pofiles on a single potemplate on productseries > * one pofile on productseries > * all the same on sourcepackages > 2. Ask for rosetta-export-queue.py script to be executed for staging > 3. Ask matsubara to look up emails that end up in the staging > mailbox starting with > 'Launchpad translation download'. For the most part, the branch looks fine. I do have a few questions and suggestions, but I'll mark this as merge-conditional on the hope that those are easy to address or answer. I'm happy to answer any questions or chat about my suggestions with you on Monday. Good job and thanks for working on this! === added file 'lib/lp/translations/emailtemplates/poexport-failure- admin-notification.txt' --- lib/lp/translations/emailtemplates/poexport-failure-admin- notification.txt 1970-01-01 00:00:00 +0000 +++ lib/lp/translations/emailtemplates/poexport-failure-admin- notification.txt 2009-08-14 16:33:57 +0000 > @@ -0,0 +1,16 @@ > +Hello admins, Maybe "Hello Launchpad administrators," ? > + > +Launchpad encountered problems exporting translation files > +requested by %(person)s (%(person_id)s) at > + > + %(request_url)s > + > +This means we have a bug in Launchpad that needs to be fixed > +before this export can proceed. Here is the error we got: > + > +%(failure_message)s > + > +%(failed_requests)s > + > +-- Note that the signature separator is actually "-- " yep with a trailing space! I know that kind of screws the no-trailing-whitespace thing, but there you have it. ;) > +Automatic message from Launchpad.net — https://launchpad.net/ === added file 'lib/lp/translations/emailtemplates/poexport-failure- unicodedecodeerror.txt' --- lib/lp/translations/emailtemplates/poexport-failure- unicodedecodeerror.txt 1970-01-01 00:00:00 +0000 +++ lib/lp/translations/emailtemplates/poexport-failure- unicodedecodeerror.txt 2009-08-14 16:33:57 +0000 > @@ -0,0 +1,12 @@ > +Hello admins, Same as above. > + > +A UnicodeDecodeError occurred while trying to notify you of a > +failure during a translation export requested by %(person)s > +(%(person_id)s) at > + > + %(request_url)s > + > +%(failed_requests)s > + > +-- > +Automatic message from Launchpad.net — https://launchpad.net/ Wow, this is like a failure failure. :) === modified file 'lib/lp/translations/scripts/po_export_queue.py' --- lib/lp/translations/scripts/po_export_queue.py 2009-07-17 00:26:05 +0000 +++ lib/lp/translations/scripts/po_export_queue.py 2009-08-14 18:02:09 +0000 > @@ -35,39 +38,210 @@ > > This class has three main attributes: > > - - name: A short identifying string for this export. > + - person: A person requesting this export. Maybe s/A/The/ ? > - url: The Librarian URL for any successfully exported files. > - - failure: Failure got while exporting. > + - failure: Failure gotten while exporting. > """ > > - def __init__(self, name): > - self.name = name > + def __init__(self, person, requested_exports, logger): > + self.person = person > self.url = None > self.failure = None > - self.object_names = [] > - > - def _getFailureEmailBody(self, person): > + self.logger = logger Is it really more convenient to access the logger through the instance rather than use the module global? I can almost promise you that it isn't noticeably faster :). > + > + self.requested_exports = list(requested_exports) > + export_requested_at = self._getExportRequestOrigin() > + self.name = self._getShortRequestName(export_requested_at) > + > + self.request_url = canonical_url( > + export_requested_at, > + rootsite='translations') + '/+export' > + > + def _getShortRequestName(self, request): > + """Return a short request name for use in email subjects.""" > + if IPOFile.providedBy(request): > + title = '%s translation of %s' % ( > + request.language.englishname, > + request.potemplate.name) > + productseries = request.potemplate.productseries > + distroseries = request.potemplate.distroseries > + sourcepackagename = request.potemplate.sourcepackagename > + elif IPOTemplate.providedBy(request): > + title = '%s template' % (request.name) > + productseries = request.productseries > + distroseries = request.distroseries > + sourcepackagename = request.sourcepackagename > + elif IProductSeries.providedBy(request): > + title = None > + productseries = request > + distroseries = None > + sourcepackagename = None > + elif ISourcePackage.providedBy(request): > + title = None > + productseries = None > + distroseries = request.distroseries > + sourcepackagename = request.sourcepackagename > + else: > + raise AssertionError( > + "We can not figure out short name for this > translation " > + "export origin.") I almost wish there was some interface and adapter for this. It's probably more work, but I'll through it out there in the off chance you think it might make the code cleaner. > + > + if productseries is not None: > + root = '%s %s' % ( > + productseries.product.displayname, > + productseries.name) > + else: Please switch this to a positive test, e.g. if productseries is None: # blah else: # blah > + root = '%s %s %s' % ( > + distroseries.distribution.displayname, > + distroseries.displayname, > + sourcepackagename.name) > + if title is not None: > + return '%s - %s' % (root, title) > + else: > + return root And this too. > + > + def _getExportRequestOrigin(self): > + """Figure out where an export request was made.""" > + # Determine all objects that export request could have > + # originated on. > + export_requested_at = None > + pofiles = set() > + implicit_potemplates = set() > + direct_potemplates = set() > + productseries = set() > + sourcepackages = set() > + > + last_template_name = None > + for request in self.requested_exports: > + if IPOTemplate.providedBy(request): > + # If we are exporting a template, add it to > + # the list of directly requested potemplates. > + potemplate = request > + direct_potemplates.add(potemplate) > + else: > + # Otherwise, we are exporting a POFile. > + potemplate = request.potemplate > + implicit_potemplates.add(potemplate) > + pofiles.add(request) > + if potemplate.displayname != last_template_name: > + self.logger.debug( > + 'Exporting objects for %s, related to template > %s' > + % (self.person.displayname, > potemplate.displayname)) > + last_template_name = potemplate.displayname > + > + # Determine productseries or sourcepackage for any > + # productseries/sourcepackage an export was requested at. > + if potemplate.productseries is not None: > + productseries.add(potemplate.productseries) > + elif potemplate.sourcepackagename is not None: > + sourcepackage = > potemplate.distroseries.getSourcePackage( > + potemplate.sourcepackagename) > + sourcepackages.add(sourcepackage) > + else: > + pass Please include a comment before the pass, explaining why that branch is empty. > + > + if len(pofiles) == 1 and len(direct_potemplates) == 0: > + # One POFile was requested. > + export_requested_at = pofiles.pop() > + elif len(pofiles) == 0 and len(direct_potemplates) == 1: > + # A POTemplate was requested. > + export_requested_at = direct_potemplates.pop() > + elif len(pofiles) + len(direct_potemplates) >= 2: > + # More than one file was requested. > + all_potemplates = > implicit_potemplates.union(direct_potemplates) > + if len(all_potemplates) == 1: > + # It's all part of a single POTemplate. > + export_requested_at = all_potemplates.pop() > + else: > + # More than one POTemplate: request was made on > + # either ProductSeries or SourcePackage. > + if len(sourcepackages) > 0: > + export_requested_at = sourcepackages.pop() > + elif len(productseries) > 0: > + export_requested_at = productseries.pop() > + > + if IPOTemplate.providedBy(export_requested_at): > + if len(sourcepackages) > 0: > + sp = sourcepackages.pop() > + if sp.getCurrentTranslationTemplates().count() == 1: > + export_requested_at = sp > + elif len(productseries) > 0: > + ps = productseries.pop() > + if ps.getCurrentTranslationTemplates().count() == 1: > + export_requested_at = ps I don't much like the abbreviations. What about using 'next' instead? > + > + return export_requested_at > + > + > + def _getRequestedExportsNames(self): Docstring. > + requested_names = [] > + for translation_object in self.requested_exports: > + if IPOTemplate.providedBy(translation_object): > + request_name = translation_object.displayname > + else: > + request_name = translation_object.title > + requested_names.append(request_name) > + > + return requested_names > + > + def _getFailureEmailBody(self): > """Send an email notification about the export failing.""" > - return textwrap.dedent(''' > - Hello %s, > - > - Launchpad encountered problems exporting the files you > requested. > - The Launchpad Translations team has been notified of > this problem. > - Please reply to this email for further assistance. > - ''' % person.displayname) > - > - def _getSuccessEmailBody(self, person): > + template = helpers.get_email_template( > + 'poexport-failure.txt', 'translations').decode('utf-8') > + return template % { > + 'person' : self.person.displayname, > + 'request_url' : self.request_url, > + } > + > + def _getFailedRequestsDescription(self): > + """Return a printable description of failed export > requests.""" > + failed_requests = self._getRequestedExportsNames() > + if len(failed_requests) > 0: > + failed_requests_text = 'Failed export request included: > \n' > + failed_requests_text += '\n'.join( > + [' * ' + request for request in failed_requests]) You actually don't need the inner square brackets. > + else: > + failed_requests_text = 'There were no export requests.' > + return failed_requests_text > + > + def _getAdminFailureNotificationEmailBody(self): > + """Send an email notification about failed export to > admins.""" > + template = helpers.get_email_template( > + 'poexport-failure-admin-notification.txt', > + 'translations').decode('utf-8') Why doesn't get_email_template() do the decoding to utf-8 automatically? When would you ever not want the email messages as a string instead of bytes? > + failed_requests = self._getFailedRequestsDescription() > + return template % { > + 'person' : self.person.displayname, > + 'person_id' : self.person.name, > + 'request_url' : self.request_url, > + 'failure_message' : self.failure, > + 'failed_requests' : failed_requests, > + } > + > + def _getUnicodeDecodeErrorEmailBody(self): > + """Send an email notification to admins about > UnicodeDecodeError.""" > + template = helpers.get_email_template( > + 'poexport-failure-unicodedecodeerror.txt', > + 'translations').decode('utf-8') > + failed_requests = self._getFailedRequestsDescription() > + return template % { > + 'person' : self.person.displayname, > + 'person_id' : self.person.name, > + 'request_url' : self.request_url, > + 'failed_requests' : failed_requests, > + } > + > + def _getSuccessEmailBody(self): > """Send an email notification about the export working.""" > - return textwrap.dedent(''' Same here, DQTQ is probably better (double quoted triple quoted string :). > - Hello %s, > - > - The translation files you requested from Launchpad are > ready for > - download from the following location: > - > - \t%s''' % (person.displayname, self.url) > - ) > - > - def notify(self, person): > + template = helpers.get_email_template( > + 'poexport-success.txt', 'translations').decode('utf-8') > + return template % { > + 'person' : self.person.displayname, > + 'download_url' : self.url, > + 'request_url' : self.request_url, > + } > + > + def notify(self): > """Send a notification email to the given person about the > export. > > If there is a failure, a copy of the email is also sent to the === added file 'lib/lp/translations/tests/helpers.py' --- lib/lp/translations/tests/helpers.py 1970-01-01 00:00:00 +0000 +++ lib/lp/translations/tests/helpers.py 2009-08-14 17:46:32 +0000 > @@ -0,0 +1,19 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +"""Helper module reused in different tests.""" > + > +__metaclass__ = type > + > +__all__ = [ > + 'print_mail_subject_and_body' > + ] > + > +import email > + > +def print_mail_subject_and_body(contents): > + msg = email.message_from_string(contents) > + body = msg.get_payload() > + print 'Subject: %s' % (msg['subject']) > + for line in body.split('\n'): > + print ">", line Why do you make the body lines look like they're being quoted?