Code review comment for lp:~wallyworld/launchpad/no-private-bug-rss

Hi Ian,

It generally looks good but as always I have some notes. And while I'm here, thanks for the little drive-by cleanups!

=== modified file 'lib/canonical/launchpad/browser/feeds.py'
--- lib/canonical/launchpad/browser/feeds.py 2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/browser/feeds.py 2011-05-20 09:01:26 +0000
@@ -177,6 +177,14 @@
             "Context %r does not provide interface %r"
             % (context, self.usedfor))

+ @classmethod
+ def feed_allowed(cls, context):
+ """Return True if a feed is allowed for the given context.

We normally use dromedaryCase for methods, where the first word is expected to be a verb. Skimming over this change at first I thought it was a property!

The "verb" part has a corollary: "feed allowed" can be misread as "feed those that are allowed." It's not hard to figure out that that's not what you mean, but it's a small distraction that can help slow down and confuse the reader.

@@ -346,4 +368,5 @@
     def feed_links(self):
         return [feed_type(self.context)
                 for feed_type in self.feed_types
- if feed_type.usedfor.providedBy(self.context)]
+ if feed_type.usedfor.providedBy(self.context) and
+ feed_type.feed_allowed(self.context)]

The indentation of that last clause threw me off a bit. Is there some more legible way to write this?

=== modified file 'lib/canonical/launchpad/pagetests/feeds/xx-links.txt'
--- lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-05 05:46:41 +0000
+++ lib/canonical/launchpad/pagetests/feeds/xx-links.txt 2011-05-20 09:01:26 +0000
@@ -42,6 +42,37 @@
         href="http://feeds.launchpad.dev/bugs/1/bug.atom"
         title="Bug 1 Feed" />]

+But if the bug is private, there should be no link.
+
+ >>> from canonical.launchpad.testing.pages import setupBrowserForUser
+ >>> from zope.component import getUtility
+ >>> from lp.registry.interfaces.person import IPersonSet
+ >>> login(ANONYMOUS)
+ >>> user = getUtility(IPersonSet).getByEmail('<email address hidden>')
+ >>> logout()
+ >>> auth_browser = setupBrowserForUser(user, 'daf')
+ >>> auth_browser.open('http://bugs.launchpad.dev/jokosher/+bug/14')
+ >>> soup = BeautifulSoup(auth_browser.contents)
+ >>> soup.head.findAll('link', type='application/atom+xml')
+ []

What exactly is auth_browser for? If you merely need a brower for a logged-in user, there's no need to construct one; use user_browser which is set up implicitly for this usage.

+But if they somehow manage to hack the url or use inline ajax editing of the
+bug status and attempt to subscribe, they are redirected to the bug page:

You're "But"-ing your own "But"!

+ >>> def print_notifications(browser):
+ ... tags = find_tags_by_class(
+ ... browser.contents, 'error message')
+ ... if len(tags) == 0:
+ ... print 'No messages.'
+ ... else:
+ ... for tag in tags:
+ ... print extract_text(tag)

We have a helper for this: get_feedback_messages.

@@ -319,3 +350,24 @@
     [<link rel="alternate" type="application/atom+xml"
         href="http://feeds.launchpad.dev/~mark/firefox/release--0.9.1/branch.atom"
  title="Latest Revisions for Branch lp://dev/~mark/firefox/release--0.9.1" />]
+
+But if the branch is private, there should be no link.

Same comments here as above!

=== modified file 'lib/lp/bugs/feed/bug.py'
--- lib/lp/bugs/feed/bug.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/feed/bug.py 2011-05-20 09:01:26 +0000

@@ -171,7 +172,15 @@
         # For a `BugFeed` we must ensure that the bug is not private.
         super(BugFeed, self).initialize()
         if self.context.private:
- raise Unauthorized("Feeds do not serve private bugs")
+ self.request.response.addErrorNotification(
+ "Feeds do not serve private bugs.")
+ if check_permission("launchpad.View", self.context):
+ self.request.response.redirect(canonical_url(self.context))
+ else:
+ # Bug cannot be seen so redirect to the bugs index page.
+ root = getUtility(ILaunchpadRoot)
+ self.request.response.redirect(
+ canonical_url(root, rootsite='bugs'))

This may be a stupid question because I'm missing some of the context, but… there's no self.feed_allowed?

I can't say I like the error message much. It isn't very clear unless the user knows exactly what they are doing. What if the bug was made private after the user loaded the page? I'd start out by saying "This branch is private" and then say something more general like "Feeds do not serve private bugs."

(Also, arguably, it may be easier to see that the user gets redirected either way if you use the if/else only for producing the redirect URL, and do the actual redirect just once afterwards.)

=== modified file 'lib/lp/code/feed/branch.py'
--- lib/lp/code/feed/branch.py 2011-03-03 01:13:47 +0000
+++ lib/lp/code/feed/branch.py 2011-05-20 09:01:26 +0000
@@ -400,8 +402,22 @@
         """See `IFeed`."""
         # For a `BranchFeed` we must ensure that the branch is not private.
         super(BranchFeed, self).initialize()
- if self.context.private:
- raise Unauthorized("Feeds do not serve private branches")
+ try:
+ feed_allowed = not self.context.private

Here I have the same comment about the error message as above (and the same question about self.feed_allowed).

But these are all minor things. Shame that it has to be so hard!

Jeroen