← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~henninge/launchpad/bug-823164-remove-translations-by into lp:launchpad

 

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

Romove the ZCA lookup from the OptionsParser and make it an explicit
step when the script is run. This makes sure that the ZCA is loaded.

== Pre-implementation notes ==

After talking to lifeless about my first proposed fix, I devised this
one which solves the problem specifically for this script.

== Implementation details ==

Since the options are being both validated and converted, I chose
"process" as a verb. Any better suggestion is welcome.

I also removed some unused code that I found.

== Tests ==

I had to expose the process_options function to be able to call it
explicitly in the tests which still assume that the conversion
happens during the construction of the script object. Looks like a
good solution to me, though.

I employed the cool "MatchesStructure" helper. Did you ever see it
in action? On failure it only lists the failed matches. Very informative.

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/translations/scripts/remove_translations.py
  lib/lp/translations/scripts/tests/test_remove_translations.py

-- 
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/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-10 16:54:37 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'process_options',
     'RemoveTranslations',
     'remove_translations',
     ]
@@ -31,8 +32,8 @@
     )
 
 
-def check_bool_option(option, opt, value):
-    """`optparse.Option` type checker for Boolean argument."""
+def process_bool_option(value):
+    """Validation and conversion for Boolean argument."""
     value = value.lower()
     bool_representations = {
         'true': True,
@@ -100,16 +101,34 @@
         return None
 
 
-def check_origin_option(option, opt, value):
-    """`optparse.Option` type checker for `RosettaTranslationsOrigin`."""
+def process_origin_option(value):
+    """Validation and conversion for `RosettaTranslationsOrigin`."""
     return get_id(value, get_origin)
 
 
-def check_person_option(option, opt, value):
-    """`optparse.Option` type checker for `Person`."""
+def process_person_option(value):
+    """Validation and conversion for `Person`."""
     return get_id(value, get_person_id)
 
 
+# Options that need special processing.
+OPTIONS_TO_PROCESS = {
+    'submitter': process_person_option,
+    'reviewer': process_person_option,
+    'origin': process_origin_option,
+    'is_current_ubuntu': process_bool_option,
+    'is_current_upstream': process_bool_option,
+    }
+
+
+def process_options(options):
+    """Process options that need special processing."""
+    for option_name, process_func in OPTIONS_TO_PROCESS.items():
+        option_value = getattr(options, option_name)
+        if option_value is not None:
+            setattr(options, option_name, process_func(option_value))
+
+
 def is_nonempty_list(list_option):
     """Is list_option a non-empty a nonempty list of option values?"""
     return list_option is not None and len(list_option) > 0
@@ -120,15 +139,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.
 
@@ -156,14 +166,6 @@
     conditions.add(match)
 
 
-class ExtendedOption(Option):
-    TYPES = Option.TYPES + ('bool', 'origin', 'person')
-    TYPE_CHECKER = dict(entry for entry in Option.TYPE_CHECKER.items())
-    TYPE_CHECKER['bool'] = check_bool_option
-    TYPE_CHECKER['origin'] = check_origin_option
-    TYPE_CHECKER['person'] = check_person_option
-
-
 class RemoveTranslations(LaunchpadScript):
     """Remove specific `TranslationMessage`s from the database.
 
@@ -178,50 +180,48 @@
     loglevel = logging.INFO
 
     my_options = [
-        ExtendedOption(
-            '-s', '--submitter', dest='submitter', type='person',
+        Option(
+            '-s', '--submitter', dest='submitter',
             help="Submitter match: delete only messages with this "
                 "submitter."),
-        ExtendedOption(
-            '-r', '--reviewer', dest='reviewer', type='person',
+        Option(
+            '-r', '--reviewer', dest='reviewer',
             help="Reviewer match: delete only messages with this reviewer."),
-        ExtendedOption(
+        Option(
             '-x', '--reject-license', action='store_true',
             dest='reject_license',
             help="Match submitters who rejected the license agreement."),
-        ExtendedOption(
+        Option(
             '-i', '--id', action='append', dest='ids', type='int',
             help="ID of message to delete.  May be specified multiple "
                 "times."),
-        ExtendedOption(
+        Option(
             '-p', '--potemplate', dest='potemplate', type='int',
             help="Template id match.  Delete only messages in this "
                 "template."),
-        ExtendedOption(
+        Option(
             '-l', '--language', dest='language',
             help="Language match.  Deletes (default) or spares (with -L) "
                  "messages in this language."),
-        ExtendedOption(
+        Option(
             '-L', '--not-language', action='store_true', dest='not_language',
             help="Invert language match: spare messages in given language."),
-        ExtendedOption(
+        Option(
             '-C', '--is-current-ubuntu', dest='is_current_ubuntu',
-            type='bool',
             help="Match on is_current_ubuntu value (True or False)."),
-        ExtendedOption(
+        Option(
             '-I', '--is-current-upstream', dest='is_current_upstream',
-            type='bool',
             help="Match on is_current_upstream value (True or False)."),
-        ExtendedOption(
+        Option(
             '-m', '--msgid', dest='msgid',
             help="Match on (singular) msgid text."),
-        ExtendedOption(
-            '-o', '--origin', dest='origin', type='origin',
+        Option(
+            '-o', '--origin', dest='origin',
             help="Origin match: delete only messages with this origin code."),
-        ExtendedOption(
+        Option(
             '-f', '--force', action='store_true', dest='force',
             help="Override safety check on moderately unsafe action."),
-        ExtendedOption(
+        Option(
             '-d', '--dry-run', action='store_true', dest='dry_run',
             help="Go through the motions, but don't really delete."),
         ]
@@ -284,6 +284,7 @@
 
     def main(self):
         """See `LaunchpadScript`."""
+        process_options(self.options)
         (result, message) = self._check_constraints_safety()
         if not result:
             raise LaunchpadScriptFailure(message)
@@ -349,6 +350,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 +431,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 'lib/lp/translations/scripts/tests/test_remove_translations.py'
--- lib/lp/translations/scripts/tests/test_remove_translations.py	2011-06-29 15:16:27 +0000
+++ lib/lp/translations/scripts/tests/test_remove_translations.py	2011-08-10 16:54:37 +0000
@@ -12,6 +12,10 @@
     OptionParser,
     OptionValueError,
     )
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+    )
 from unittest import TestLoader
 
 from storm.store import Store
@@ -34,6 +38,7 @@
     TranslationRelicensingAgreement,
     )
 from lp.translations.scripts.remove_translations import (
+    process_options,
     remove_translations,
     RemoveTranslations,
     )
@@ -60,7 +65,9 @@
 
     def _check_options(self, opts):
         """Get `_check_constraints_safety`'s answer for given options."""
-        return make_script(opts)._check_constraints_safety()
+        script = make_script(opts)
+        process_options(script.options)
+        return script._check_constraints_safety()
 
     def test_RecklessRemoval(self):
         # The script will refuse to run if no specific person or id is
@@ -157,6 +164,7 @@
     parser = OptionChecker()
     parser.add_options(RemoveTranslations.my_options)
     options, arguments = parser.parse_args(args=opts)
+    process_options(options)
     return options
 
 
@@ -185,17 +193,17 @@
             '--origin=1',
             '--force',
             ])
-        self.assertEqual(options.submitter, 1)
-        self.assertEqual(options.reviewer, 2)
-        self.assertEqual(options.ids, [3, 4])
-        self.assertEqual(options.potemplate, 5)
-        self.assertEqual(options.language, 'te')
-        self.assertEqual(options.not_language, True)
-        self.assertEqual(options.is_current_ubuntu, True)
-        self.assertEqual(options.is_current_upstream, False)
-        self.assertEqual(options.is_current_upstream, False)
-        self.assertEqual(options.origin, 1)
-        self.assertEqual(options.force, True)
+        self.assertThat(options, MatchesStructure(
+            submitter=Equals(1),
+            reviewer=Equals(2),
+            ids=Equals([3, 4]),
+            potemplate=Equals(5),
+            language=Equals('te'),
+            not_language=Equals(True),
+            is_current_ubuntu=Equals(True),
+            is_current_upstream=Equals(False),
+            origin=Equals(1),
+            force=Equals(True)))
 
     def test_WithLookups(self):
         # The script can also look up some items from different
@@ -211,11 +219,12 @@
             '--is-current-upstream=true',
             '--origin=SCM',
             ])
-        self.assertEqual(options.submitter, submitter.id)
-        self.assertEqual(options.reviewer, reviewer.id)
-        self.assertEqual(options.is_current_ubuntu, False)
-        self.assertEqual(options.is_current_upstream, True)
-        self.assertEqual(options.origin, RosettaTranslationOrigin.SCM.value)
+        self.assertThat(options, MatchesStructure(
+            submitter=Equals(submitter.id),
+            reviewer=Equals(reviewer.id),
+            is_current_ubuntu=Equals(False),
+            is_current_upstream=Equals(True),
+            origin=Equals(RosettaTranslationOrigin.SCM.value)))
 
     def test_BadBool(self):
         self.assertRaises(Exception, parse_opts, '--is-current-ubuntu=None')


Follow ups