Barry, thanks for the review. I've applied most of your recommendations. The ones I didn't were about 'positive tests' (i.e. is 'is not None' really a negative test?) and I haven't provided unit tests. Still, I've modified doctests to use mail_helpers, so that resulted in a nice big incremental diff. У пет, 14. 08 2009. у 16:05 -0400, Barry Warsaw пише: > > * 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? Yes, I've looked through existing helpers. I've already struggled to keep the diff size reasonable, and this would have made it only worse. Though, I am happy to migrate existing tests to the use them, and I did so. Note that only this change means 640 lines of diff in the incremental diff ;) > > 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. Thanks. > 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. Great, thanks. > === 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," ? Sure, fixed. Note that these emails only ended up on our internal error-reports list, but no reason not to make them even nicer :) > > +-- > > 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. ;) Ah, interesting. Fixed all over the place. > > +Automatic message from Launchpad.net — https://launchpad.net/ > > Wow, this is like a failure failure. :) My mail client linkifies the second, but not the first. I still prefer the former in an email message, so I made it "Automatic message from Launchpad.net." in all the email templates. > === 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/ ? Sure, I'll trust an American on this more than I'd trust a Serbian :) > > - 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 :). Considering how tests use MockLogger for whatever reason, I ain't changing this bit right now either. I fully expect it to be slower, but there are more important things for us to work on than fix every single problem in our ancient tests :) > > + 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. It's noticably more work for one single place we'd use it in. As mentioned before, this is only so we can use the best subject (or well, much improved one over what the existing case is) in our 'download available' notification emails. > > + > > + if productseries is not None: > > + root = '%s %s' % ( > > + productseries.product.displayname, > > + productseries.name) > > + else: > > Please switch this to a positive test, e.g. Heh, I'd actually call this a "positive" test, since I am negating a comparison to None. It made infinitely more sense to me when re-reading the code, because it's obvious that I can use productseries in the branch. > > + 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. I'd put out the same argument here as above. I.e. I am using `title` only where I am certain it's not None. Being explicit (instead of relying on an 'else' of 'is None' check) seems to make more sense to me. > > + # 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. I've actually made it an assertion instead. > > + 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? Good catch, they are left-overs from when I was prototyping the code in 'make harness'. And, using a variable there (I used 'container') allows me to simplify the code further (getCurrentTranslationTemplates() call is identical on either SourcePackage or ProductSeries). > > + > > + return export_requested_at > > + > > + > > + def _getRequestedExportsNames(self): > > Docstring. Added. > > + 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. Fixed. > > + 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? I have no idea, and I am not willing to fix it in this branch and risk who knows what failing tests I might run into. :) Instead, I've removed the decoding since I removed the em-dash I used in my lovely signature you didn't like: "Automatic email from Launchpad.net https://launchpad.net/" :) > > + 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 :). I guess you mistakenly haven't noticed that this is the code that is being removed to be replaced by an external email template :) > === 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? That was to reduce the diff size, because that's exactly how previous test did it. But I am happy to report that you suggested me to use print_emails instead, so this file is entirely gone now :) Incremental diff attached, I believe I've addressed all of your concerns. Cheers, Danilo