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

Yello

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

I always appreciate the thoroughness of your reviews.

>
>
> 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.
>

Good point. Will fix.

>
>
> @@ -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?
>

I'll find something.

>
> === 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.
>

I'll take another look. There's already a "browser" instance which is
used for (I think) non-authenticated access but I didn't realise there
was already a "user_browser" instance.

>
> +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.
>

Really? I copied the above code from elsewhere :-)
I'll fix it.

>
> === 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?

It's not needed here. The above is for the BudFeed class. This bit above
is to catch the case where user manages to bypass the disabled rss
button via url hacking or inline ajax editing. The (soon to be renamed)
feed_allowed is used in the BugFeedLink class to prevent the rss button
from being enabled as the page is loaded.

>
> 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."
>

Ok. I'll reword it. I just used whatever wording was already being used
for the Unauthorised exception.

> (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.)
>
> But these are all minor things. Shame that it has to be so hard!
>

I don't mind. I'm grateful for the comments and am pleased to be able to
produce better code.