launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/urlparse-does-not-make-me-happy into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #823471 in Launchpad itself: "privacy ribbon does not appear on secondary sites"
  https://bugs.launchpad.net/launchpad/+bug/823471

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/urlparse-does-not-make-me-happy/+merge/82415

Summary
=======
A previous branch was landed to allow our launchpad_loggerhead tools to ping the API to determine if a privacy notification should be shown on loggerhead. That branch used urlparse.urljoin to construct the API url, which resulted in the wrong url being created, as demonstrated by William Grant.

This branch does a bit of cleanup and adds some robustness to the privacy code, as well as addressing the bad url construction.

Preimp
======
Spoke with William Grant, Curtis Hovey

Implementation
==============
Instead of using urlparse.urljoin, this branch just manually builds the url from the components.

It also adds 401 to the list of return codes that implies a private branch, and closes the response once it's done checking the status.

QA
==
Go to a private branch; when opening it in loggerhead, the privacy ribbon should display. When opening a pubic branch in loggerhead, it should not.

Lint
====
This branch is lint free.
-- 
https://code.launchpad.net/~jcsackett/launchpad/urlparse-does-not-make-me-happy/+merge/82415
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/urlparse-does-not-make-me-happy into lp:launchpad.

=== modified file 'lib/launchpad_loggerhead/app.py'
--- lib/launchpad_loggerhead/app.py	2011-10-26 16:21:36 +0000
+++ lib/launchpad_loggerhead/app.py	2011-11-16 15:47:33 +0000
@@ -238,8 +238,12 @@
             if not os.path.isdir(cachepath):
                 os.makedirs(cachepath)
             self.log.info('branch_url: %s', branch_url)
-            branch_api_url = urlparse.urljoin(
-                config.appserver_root_url('api'), 'devel', branch_name)
+            base_api_url = config.appserver_root_url('api')
+            branch_api_url = '%s/%s/%s' % (
+                base_api_url,
+                'devel',
+                branch_name,
+                )
             self.log.info('branch_api_url: %s', branch_api_url)
             req = urllib2.Request(branch_api_url)
             private = False
@@ -247,16 +251,19 @@
                 # We need to determine if the branch is private
                 response = urllib2.urlopen(req)
             except urllib2.HTTPError as response:
-                if response.getcode() in (400, 403):
-                    ## 400 and 403 are the possible returns for api requests
-                    ## on a private branch without authentication.
+                code = response.getcode()
+                if code in (400, 401, 403):
+                    # 400, 401, and 403 are the possible returns for api
+                    # requests on a private branch without authentication.
                     self.log.info("Branch is private")
                     private = True
                 self.log.info(
                     "Branch state not determined; api error, return code: %s",
-                    response.getcode())
+                    code)
+                response.close()
             else:
                 self.log.info("Branch is public")
+                response.close()
 
             try:
                 bzr_branch = safe_open(