launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/bug-823164-remove-translations-by into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #823164 in Launchpad itself: "remove-translations-by script fails to lookup user names"
  https://bugs.launchpad.net/launchpad/+bug/823164

For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-823164-remove-translations-by/+merge/70961

= Bug 823164 =

The remove-translations-by script fails when trying to look up the id
for a person name passed in on the command lilne because the Zope
Component Architicture has not been loaded yet that point. It does not
get loaded until the script is actually run.

== Proposed fix ==

Add an option to load the ZCA early, before command line options are
being parsed.

== Pre-implementation notes ==

None but this seems pretty obvious. It might be worth considering if
loading the ZCA early should become the default but I did not want to
put in the effort to check what that might break.

== Implementation details ==

When loading the ZCA early, the use_web_security parameter defaults to
False. I did not want to add another parameter to __init__ since I don't
need to change that parameter for this script.
 
I also removed some unused code that I found.

== Tests ==

The test succeeds even without the early_zca parameter set to True
because execute_zcml_for_scripts is already called by the test layer.
There is no fitting test layer at the moment, so we'll have to live
with this.

bin/test -vvcm lp.translations.scripts.tests.test_remove_translations

== Demo and Q/A ==

scripts/rosetta/remove-translations-by.py -d -s henninge

If that succeeds without an error, the bug is fixed. Don't remove the -d
switch on production or else you will cause damage. But QA should happen
on staging anyway...

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/scripts/base.py
  lib/lp/translations/scripts/remove_translations.py
  scripts/rosetta/remove-translations-by.py

./scripts/rosetta/remove-translations-by.py
      12: '_pythonpath' imported but unused
-- 
https://code.launchpad.net/~henninge/launchpad/bug-823164-remove-translations-by/+merge/70961
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-823164-remove-translations-by into lp:launchpad.

=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py	2011-08-02 23:42:08 +0000
+++ lib/lp/services/scripts/base.py	2011-08-09 21:35:34 +0000
@@ -148,7 +148,8 @@
     # State for the log_unhandled_exceptions decorator.
     _log_unhandled_exceptions_level = 0
 
-    def __init__(self, name=None, dbuser=None, test_args=None, logger=None):
+    def __init__(self, name=None, dbuser=None,
+                 test_args=None, logger=None, early_zca=False):
         """Construct new LaunchpadScript.
 
         Name is a short name for this script; it will be used to
@@ -162,6 +163,8 @@
 
         :param logger: Use this logger, instead of initializing global
             logging.
+        :param early_zca: Register the ZCA early, before parsing options.
+            If False, registration is delayed until the script is run.
         """
         if name is None:
             self._name = self.__class__.__name__.lower()
@@ -170,6 +173,7 @@
 
         self._dbuser = dbuser
         self.logger = logger
+        self._early_zca = early_zca
 
         # The construction of the option parser is a bit roundabout, but
         # at least it's isolated here. First we build the parser, then
@@ -180,6 +184,8 @@
             description = self.__doc__
         else:
             description = self.description
+        if self._early_zca:
+            self._init_zca(use_web_security=False)
         self.parser = OptionParser(usage=self.usage,
                                    description=description)
 
@@ -315,7 +321,8 @@
 
         if isolation is None:
             isolation = ISOLATION_LEVEL_DEFAULT
-        self._init_zca(use_web_security=use_web_security)
+        if not self._early_zca:
+            self._init_zca(use_web_security=use_web_security)
         self._init_db(isolation=isolation)
 
         date_started = datetime.datetime.now(UTC)

=== modified file 'lib/lp/translations/scripts/remove_translations.py'
--- lib/lp/translations/scripts/remove_translations.py	2011-02-03 15:45:34 +0000
+++ lib/lp/translations/scripts/remove_translations.py	2011-08-09 21:35:34 +0000
@@ -120,15 +120,6 @@
     return string_option is not None and string_option != ''
 
 
-def check_option_type(options, option_name, option_type):
-    """Check that option value is of given type, or None."""
-    option = getattr(options, option_name)
-    if option is not None and not isinstance(option, option_type):
-        raise ValueError(
-            "Wrong argument type for %s: expected %s, got %s." % (
-                option_name, option_type.__name__, option.__class__.__name__))
-
-
 def compose_language_match(language_code):
     """Compose SQL condition for matching a language in the deletion query.
 
@@ -349,6 +340,7 @@
                     'Message %i is a current translation in %s'
                     % (id, ' and '.join(current)))
 
+
 def remove_translations(logger=None, submitter=None, reviewer=None,
                         reject_license=False, ids=None, potemplate=None,
                         language_code=None, not_language=False,
@@ -429,11 +421,6 @@
 
     assert len(conditions) > 0, "That would delete ALL translations, maniac!"
 
-    if len(joins) > 0:
-        using_clause = 'USING %s' % ', '.join(joins)
-    else:
-        using_clause = ''
-
     cur = cursor()
     drop_tables(cur, 'temp_doomed_message')
 

=== modified file 'scripts/rosetta/remove-translations-by.py'
--- scripts/rosetta/remove-translations-by.py	2010-04-27 19:48:39 +0000
+++ scripts/rosetta/remove-translations-by.py	2011-08-09 21:35:34 +0000
@@ -17,5 +17,5 @@
 if __name__ == '__main__':
     script = RemoveTranslations(
         'canonical.launchpad.scripts.remove-translations',
-        dbuser='rosettaadmin')
+        dbuser='rosettaadmin', early_zca=True)
     script.run()