launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/translate-path-private into lp:launchpad.

Commit message:
Make the translatePath XML-RPC methods for Bazaar and Git tell the caller whether the branch/repository is private.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/translate-path-private/+merge/260128

Make the translatePath XML-RPC methods for Bazaar and Git tell the caller whether the branch/repository is private.

This will let us implement a privacy banner for cgit without awful hacks, and eradicate said awful hacks from the loggerhead app in LP.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/translate-path-private into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2015-05-13 08:44:04 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2015-05-26 11:01:08 +0000
@@ -316,7 +316,7 @@
             writable = self._canWriteToBranch(requester, branch)
         return (
             BRANCH_TRANSPORT,
-            {'id': branch_id, 'writable': writable},
+            {'id': branch_id, 'writable': writable, 'private': branch.private},
             trailing_path)
 
     def _serializeControlDirectory(self, requester, lookup):

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2015-05-07 09:46:22 +0000
+++ lib/lp/code/xmlrpc/git.py	2015-05-26 11:01:08 +0000
@@ -84,6 +84,7 @@
             "path": hosting_path,
             "writable": writable,
             "trailing": extra_path,
+            "private": repository.private,
             }
 
     def _getGitNamespaceExtras(self, path, requester):

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2015-05-13 08:44:04 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2015-05-26 11:01:08 +0000
@@ -512,7 +512,8 @@
         login(ANONYMOUS)
         translation = self.codehosting_api.translatePath(owner.id, path)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch_id, 'writable': True}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch_id, 'writable': True, 'private': False}, ''),
             translation)
 
     def test_createBranch_using_branch_alias_product(self):
@@ -540,7 +541,8 @@
         login(ANONYMOUS)
         translation = self.codehosting_api.translatePath(owner.id, path)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch_id, 'writable': True}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch_id, 'writable': True, 'private': False}, ''),
             translation)
 
     def test_createBranch_using_branch_alias_product_not_auth(self):
@@ -757,7 +759,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_branch_with_trailing_slash(self):
@@ -767,7 +770,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_path_in_branch(self):
@@ -777,7 +781,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, 'child'),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, 'child'),
             translation)
 
     def test_translatePath_nested_path_in_branch(self):
@@ -787,7 +792,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, 'a/b'),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, 'a/b'),
             translation)
 
     def test_translatePath_preserves_escaping(self):
@@ -802,7 +808,7 @@
         login(ANONYMOUS)
         self.assertEqual(
             (BRANCH_TRANSPORT,
-             {'id': branch.id, 'writable': False},
+             {'id': branch.id, 'writable': False, 'private': False},
              escape(child_path)), translation)
 
     def test_translatePath_no_such_junk_branch(self):
@@ -839,7 +845,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': True}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': True, 'private': True}, ''),
             translation)
 
     def test_translatePath_cant_see_private_branch(self):
@@ -863,7 +870,8 @@
             LAUNCHPAD_SERVICES, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': True}, ''),
             translation)
 
     def test_translatePath_anonymous_cant_see_private_branch(self):
@@ -878,7 +886,8 @@
         translation = self.codehosting_api.translatePath(
             LAUNCHPAD_ANONYMOUS, path)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_owned(self):
@@ -889,7 +898,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': True}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': True, 'private': False}, ''),
             translation)
 
     def test_translatePath_team_owned(self):
@@ -901,7 +911,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': True}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': True, 'private': False}, ''),
             translation)
 
     def test_translatePath_team_unowned(self):
@@ -913,7 +924,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_owned_mirrored(self):
@@ -924,7 +936,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_owned_imported(self):
@@ -935,7 +948,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_branch_alias_short_name(self):
@@ -951,7 +965,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False},
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False},
              path_in_branch), translation)
 
     def test_translatePath_branch_alias_unique_name(self):
@@ -965,7 +980,8 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         login(ANONYMOUS)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False},
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False},
              path_in_branch), translation)
 
     def test_translatePath_branch_alias_no_such_branch(self):
@@ -1080,7 +1096,7 @@
         translation = self.codehosting_api.translatePath(requester.id, path)
         expected = (
             BRANCH_TRANSPORT,
-            {'id': branch.id, 'writable': False},
+            {'id': branch.id, 'writable': False, 'private': False},
             'foo/bar',
             )
         self.assertEqual(expected, translation)
@@ -1094,7 +1110,8 @@
         path = escape(branch_id_alias(branch))
         translation = self.codehosting_api.translatePath(requester.id, path)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': False}, ''),
             translation)
 
     def test_translatePath_branch_id_alias_private_branch(self):
@@ -1108,7 +1125,8 @@
         path = escape(branch_id_alias(branch))
         translation = self.codehosting_api.translatePath(requester.id, path)
         self.assertEqual(
-            (BRANCH_TRANSPORT, {'id': branch.id, 'writable': False}, ''),
+            (BRANCH_TRANSPORT,
+             {'id': branch.id, 'writable': False, 'private': True}, ''),
             translation)
 
     def test_translatePath_branch_id_alias_private_branch_no_access(self):

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2015-05-14 13:57:51 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2015-05-26 11:01:08 +0000
@@ -137,7 +137,7 @@
 
     def assertTranslates(self, requester, path, repository, writable,
                          permission="read", can_authenticate=False,
-                         trailing=""):
+                         trailing="", private=False):
         if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
             requester = requester.id
         translation = self.git_api.translatePath(
@@ -145,10 +145,11 @@
         login(ANONYMOUS)
         self.assertEqual(
             {"path": repository.getInternalPath(), "writable": writable,
-             "trailing": trailing},
+             "trailing": trailing, "private": private},
             translation)
 
-    def assertCreates(self, requester, path, can_authenticate=False):
+    def assertCreates(self, requester, path, can_authenticate=False,
+                      private=False):
         if requester in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
             requester_id = requester
         else:
@@ -162,7 +163,7 @@
         self.assertEqual(requester, repository.registrant)
         self.assertEqual(
             {"path": repository.getInternalPath(), "writable": True,
-             "trailing": ""},
+             "trailing": "", "private": private},
             translation)
         self.assertEqual(
             ("create", repository.getInternalPath()),
@@ -182,7 +183,7 @@
             self.factory.makeGitRepository(
                 owner=requester, information_type=InformationType.USERDATA))
         path = u"/%s" % repository.unique_name
-        self.assertTranslates(requester, path, repository, True)
+        self.assertTranslates(requester, path, repository, True, private=True)
 
     def test_translatePath_cannot_see_private_repository(self):
         requester = self.factory.makePerson()

=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2012-10-31 19:11:51 +0000
+++ lib/lp/codehosting/inmemory.py	2015-05-26 11:01:08 +0000
@@ -240,6 +240,10 @@
         self.sourcepackagename = sourcepackagename
 
     @property
+    def private(self):
+        return self.information_type in PRIVATE_INFORMATION_TYPES
+
+    @property
     def unique_name(self):
         if self.product is None:
             if self.distroseries is None:
@@ -834,7 +838,7 @@
             writable = self._canWrite(requester_id, branch)
         return (
             BRANCH_TRANSPORT,
-            {'id': branch.id, 'writable': writable},
+            {'id': branch.id, 'writable': writable, 'private': branch.private},
             trailing_path)
 
     def performLookup(self, requester_id, lookup, branch_name_only=False):