Merge lp:~mars/tarmac/add-branch-config-get into lp:~launchpad/tarmac/lp-tarmac

[Māris Fogels]

Adds a get() method to the BranchConfig object and fixes allowedcontributors.py plugin to handle private teams.

Hello,

This branch adds a get() method to the BranchConfig object. This makes tests for keys in a BranchConfig easier to write. You no longer have to mess around with hasattr() and conditional statements for optional BranchConfig keys (for example, to conditionally activate a plugin based on a branch config key's presence).

The implementation is currently a simple wrapper for getattr(). Later we may want to proxy a proper dict object, but that raises the problem of clashes between the dict method names and the config object key names. It is not something we need yet.

Thanks,

Maris

[Māris Fogels (mars)]

Self-approved for the Launchpad Tarmac trial and landing on the lp-tarmac fork.

review: Approve

=== modified file 'tarmac/config.py'
--- tarmac/config.py	2010-10-12 17:52:27 +0000
+++ tarmac/config.py	2010-12-16 02:29:48 +0000
@@ -125,3 +125,10 @@
         if config.has_section(branch_name):
             for key, val in config.items(branch_name):
                 setattr(self, key, val)
+
+    def get(self, attr, default=None):
+        '''A convenient method for getting a config key that may be missing.
+
+        Defaults to None if the key is not set.
+        '''
+        return getattr(self, attr, default)

=== modified file 'tarmac/plugins/allowedcontributors.py'
--- tarmac/plugins/allowedcontributors.py	2010-09-20 18:25:54 +0000
+++ tarmac/plugins/allowedcontributors.py	2010-12-16 02:29:48 +0000
@@ -23,6 +23,10 @@
     """Error for when a contributor does not meet validation requirements."""
 
 
+class InvalidPersonOrTeam(TarmacMergeError):
+    """Error for when a required team could not be found."""
+
+
 class AllowedContributors(TarmacPlugin):
     """Tarmac plug-in for checking whether a contributor is allowed to.
 
@@ -52,9 +56,25 @@
                 in_team = False
                 for team in self.allowed_contributors:
                     launchpad = command.launchpad
-                    lp_members = launchpad.people[team].getMembersByStatus(
-                        status=[u'Approved', u'Administrator'])
-                    members = [x.name for x in lp_members]
+                    try:
+                        lp_members = launchpad.people[team].getMembersByStatus(
+                            status=u'Approved')
+                        lp_members.extend(
+                            launchpad.people[team].getMembersByStatus(
+                                status=u'Administrator'))
+                        members = [x.name for x in lp_members]
+                    except KeyError:
+                        message = (u'Could not find person or team "%s" on '
+                                   u'Launchpad.' % team)
+                        comment = (u'Merging into %(target) requires that '
+                                   u'contributing authors be a member of an '
+                                   u'acceptable team, or a specified person. '
+                                   u'However, the person or team "%(team)s" '
+                                   u'was not found on Launchpad.' % {
+                                'target': proposal.target_branch.display_name,
+                                'team': team})
+                        raise InvalidPersonOrTeam(message, comment)
+
                     if author in members:
                         in_team = True
                         break

=== modified file 'tarmac/plugins/tests/test_allowedcontributors.py'
--- tarmac/plugins/tests/test_allowedcontributors.py	2010-09-20 18:25:54 +0000
+++ tarmac/plugins/tests/test_allowedcontributors.py	2010-12-16 02:29:48 +0000
@@ -16,7 +16,7 @@
 """Tests for the Allowed Contributors plug-in."""
 
 from tarmac.plugins.allowedcontributors import (
-    InvalidContributor, AllowedContributors)
+    InvalidContributor, InvalidPersonOrTeam, AllowedContributors)
 from tarmac.tests import TarmacTestCase
 from tarmac.tests.mock import Thing
 
@@ -47,7 +47,7 @@
                    Thing(name=u'person3', status=u'Approved'),
                    Thing(name=u'person8', status=u'Invited'),
                    Thing(name=u'personX', status=u'Expired')]
-        return [x for x in members if x.status in status]
+        return [x for x in members if x.status == status]
 
     def noMembersByStatus(self, status=None):
         """Fake method to not return any members for people."""
@@ -74,3 +74,16 @@
                           self.plugin.run,
                           command=command, target=target, source=source,
                           proposal=self.proposal)
+
+    def test_run_private_team(self):
+        """Test that the plug-in runs correctly."""
+        config = Thing(allowed_contributors=u'private_team')
+        source = Thing(authors=[u'person1', u'person2', u'person3'])
+        target = Thing(config=config)
+        launchpad = Thing(people=self.people)
+        command = Thing(launchpad=launchpad)
+        self.assertRaises(InvalidPersonOrTeam,
+                          self.plugin.run,
+                          command=command, target=target, source=source,
+                          proposal=self.proposal)
+

=== modified file 'tarmac/tests/test_config.py'
--- tarmac/tests/test_config.py	2010-10-12 17:52:27 +0000
+++ tarmac/tests/test_config.py	2010-12-16 02:29:48 +0000
@@ -61,3 +61,23 @@
         self.assertRaises(NoOptionError,
                           self.config.get, 'test', 'test_option')
         self.config.remove_section('test')
+
+
+class BranchConfigTestCase(TarmacTestCase):
+    '''Tests for the tarmac.config.BranchConfig object.'''
+
+    def test_get_value(self):
+        expected_value = 'test value'
+        self.config.add_section('lp:test_get_value')
+        self.config.set('lp:test_get_value', 'test_key', expected_value)
+
+        config = BranchConfig('lp:test_get_value', self.config)
+
+        self.assertTrue(hasattr(config, 'test_key'))
+        self.assertEqual(config.test_key, expected_value)
+        self.assertEqual(config.get('test_key'), expected_value)
+
+    def test_get_unset_value_returns_none(self):
+        config = BranchConfig('lp:test_no_keys', self.config)
+        self.assertFalse(hasattr(config, 'missing_key'))
+        self.assertIs(None, config.get('missing_key'))