Launchpad itself

Code review comment for lp:~danilo/launchpad/bug-410579

bug-410579
Merge into devel

Revision history for this message

Данило Шеган (danilo) wrote on 2009-08-14:

= Summary =

This branch fixes #92751 (#410579 is a duplicate which I originally
started on), doing a bunch of drive-by cleanups along the way.

Our current download email for translations is very terse and not very
informative. It goes something like the following:

Subject: Translation download request: danilo
> Hello Данило Шеган,
>
> The translation files you requested from Launchpad are ready for
> download from the following location:
>
> http://launchpadlibrarian.net/30048422/po_kio4-es.po

== Proposed fix ==

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.

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.

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).

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.

* interfaces/translationimportqueue.py
Drive-by fix for #406799 (s/imported imported/imported/).

== 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'.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/doc/poexport-request.txt
  lib/lp/translations/doc/poexport-queue.txt
  lib/lp/translations/emailtemplates/poexport-failure-unicodedecodeerror.txt
  lib/lp/translations/tests/helpers.py
  lib/lp/translations/scripts/po_export_queue.py
  lib/lp/translations/emailtemplates/poexport-failure-admin-notification.txt
  lib/lp/translations/doc/poexport-request-productseries.txt
  lib/lp/translations/emailtemplates/poexport-success.txt
  lib/lp/translations/emailtemplates/poexport-failure.txt
  lib/lp/translations/interfaces/translationimportqueue.py

= Summary =

This branch fixes #92751 (#410579 is a duplicate which I originally
started on), doing a bunch of drive-by cleanups along the way.

Our current download email for translations is very terse and not very
informative.  It goes something like the following:

Subject: Translation download request: danilo
> Hello Данило Шеган,
> 
> The translation files you requested from Launchpad are ready for
> download from the following location:
>
>    http://launchpadlibrarian.net/30048422/po_kio4-es.po

== Proposed fix ==

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.

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.

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).

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.

* interfaces/translationimportqueue.py
   Drive-by fix for #406799 (s/imported imported/imported/).