launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-348996 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #348996 in Launchpad itself: "attachments page fails to redirect to +attachment and oopses"
  https://bugs.launchpad.net/launchpad/+bug/348996

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-348996/+merge/60612

canonical.launchpad.webapp.publisher.redirection() returns a RedirectionView which cannot handle a subpath, causing an OOPS if a redirection() is traversed through. This branch fixes all offending callers to use self.redirectSubTree instead, which handles a subpath by appending it to the redirection URL.

I added tests for cases which have bugs reported (Bug(Task)/attachments, bug #348996), but decided it wasn't really worth bloating notfound-traversals.txt with identical tests for the rest. They OOPS extremely rarely on production, and aren't very likely to ever be touched again.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-348996/+merge/60612
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-348996 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt'
--- lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2011-03-25 03:26:29 +0000
+++ lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2011-05-11 11:02:12 +0000
@@ -274,6 +274,7 @@
 >>> check_not_found("/firefox/+bug/1/attachments")
 >>> check_not_found("/firefox/+bug/1/+attachment")
 >>> check_redirect("/firefox/+bug/1/attachments/1", status=301)
+>>> check_redirect("/firefox/+bug/1/attachments/1/+edit", status=301)
 >>> check("/firefox/+bug/1/+attachment/1", auth=True)
 
 Bug attachments in the context of a bug are all redirected to be at
@@ -282,6 +283,7 @@
 >>> check_not_found("/bugs/2/attachments")
 >>> check_not_found("/bugs/2/+attachment")
 >>> check_redirect("/bugs/1/attachments/1", status=301)
+>>> check_redirect("/bugs/1/attachments/1/+edit", status=301)
 >>> check("/bugs/1/+attachment/1", auth=True)
 
 If a requested bug attachment is not associated with the bug already

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2011-05-09 22:53:33 +0000
+++ lib/lp/answers/browser/question.py	2011-05-11 11:02:12 +0000
@@ -74,7 +74,6 @@
     Link,
     Navigation,
     NavigationMenu,
-    redirection,
     )
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
@@ -244,7 +243,7 @@
         if hasattr(self.request, 'version'):
             return question
         else:
-            return redirection(
+            return self.redirectSubTree(
                 canonical_url(question, self.request), status=301)
 
 

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-05-10 10:05:56 +0000
+++ lib/lp/bugs/browser/bug.py	2011-05-11 11:02:12 +0000
@@ -66,7 +66,6 @@
     LaunchpadView,
     Link,
     Navigation,
-    redirection,
     StandardLaunchpadFacets,
     stepthrough,
     structured,
@@ -146,7 +145,8 @@
         if name.isdigit():
             attachment = getUtility(IBugAttachmentSet)[name]
             if attachment is not None and attachment.bug == self.context:
-                return redirection(canonical_url(attachment), status=301)
+                return self.redirectSubTree(
+                    canonical_url(attachment), status=301)
 
     @stepthrough('+attachment')
     def traverse_attachment(self, name):
@@ -438,7 +438,7 @@
             #      If fixed_bugtasks isn't sliced, it will take a long time
             #      to iterate over it, even over just 10, because
             #      Transaction.iterSelect() listifies the result.
-            for bugtask in fixed_bugtasks[:4*limit]:
+            for bugtask in fixed_bugtasks[:4 * limit]:
                 if bugtask.bug not in fixed_bugs:
                     fixed_bugs.append(bugtask.bug)
                     if len(fixed_bugs) >= limit:

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-04-22 12:46:12 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-05-11 11:02:12 +0000
@@ -363,7 +363,7 @@
             # this comment has a new title, so make that the rolling focus
             current_title = comment.title
             comment.display_title = True
-    if for_display and comments and comments[0].index==0:
+    if for_display and comments and comments[0].index == 0:
         # We show the text of the first comment as the bug description,
         # or via the special link "View original description", but we want
         # to display attachments filed together with the bug in the
@@ -565,7 +565,8 @@
         if name.isdigit():
             attachment = getUtility(IBugAttachmentSet)[name]
             if attachment is not None and attachment.bug == self.context.bug:
-                return redirection(canonical_url(attachment), status=301)
+                return self.redirectSubTree(
+                    canonical_url(attachment), status=301)
 
     @stepthrough('+attachment')
     def traverse_attachment(self, name):
@@ -774,13 +775,13 @@
                < config.malone.comments_list_max_length)
 
         if not self.visible_comments_truncated_for_display:
-            comments=self.comments
+            comments = self.comments
         else:
             # the comment function takes 0-offset counts where comment 0 is
             # the initial description, so we need to add one to the limits
             # to adjust.
             oldest_count = 1 + self.visible_initial_comments
-            new_count = 1 + self.total_comments-self.visible_recent_comments
+            new_count = 1 + self.total_comments - self.visible_recent_comments
             comments = get_comments_for_bugtask(
                 self.context, truncate=True, for_display=True,
                 slice_info=[
@@ -2990,7 +2991,8 @@
             (True, bug_listing_item.review_action_widget)
             for bug_listing_item in self.context.getBugListingItems()
             if bug_listing_item.review_action_widget is not None]
-        self.widgets = formlib.form.Widgets(widgets_list, len(self.prefix)+1)
+        self.widgets = formlib.form.Widgets(
+            widgets_list, len(self.prefix) + 1)
 
     @action('Save changes', name='submit', condition=canApproveNominations)
     def submit_action(self, action, data):
@@ -3150,7 +3152,7 @@
             self.milestones = list(
                 bugtask_set.getBugTaskTargetMilestones(self.bugtasks))
         else:
-            self.milestones = []    
+            self.milestones = []
         distro_packages = defaultdict(list)
         distro_series_packages = defaultdict(list)
         for bugtask in self.bugtasks:

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2011-04-13 14:37:51 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2011-05-11 11:02:12 +0000
@@ -72,7 +72,6 @@
     canonical_url,
     GetitemNavigation,
     Link,
-    redirection,
     StandardLaunchpadFacets,
     stepto,
     )
@@ -189,7 +188,7 @@
             distro_sourcepackage, view_name='+filebug')
         if self.request.form.get('no-redirect') is not None:
             redirection_url += '?no-redirect'
-        return redirection(redirection_url)
+        return self.redirectSubTree(redirection_url)
 
 
 @adapter(ISourcePackage)