Code review comment for lp:~brian-murray/launchpad/limited-subscriptions-page

Hi Brian,

I like this branch; nice work. Couple of minor changes needed, but
nothing that isn't just nitpicking. I've also posed a question about
performance, but I'm not sure that we can do much about at this point.

You should get a UI review for this branch from one of our sainted UI
reviewers before you land it.

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2010-09-10 12:24:03 +0000
> +++ lib/lp/registry/browser/person.py 2010-09-10 22:30:12 +0000
> @@ -2396,6 +2397,42 @@
> return self.getSearchPageHeading()
>
>
> +class PersonSubscriptionsView(BugTaskSearchListingView):
> + """All the subscriptions for a person."""
> +
> + page_title = 'Subscriptions'
> +
> + def subscribedBugTasks(self):
> + """Return a BatchNavigator for distinct bug tasks to which the
> + person is subscribed."""
> + bugtasks = self.context.searchTasks(None, user=self.user,

bugtasks => bug_tasks.

> + order_by='-date_last_updated',
> + status=(BugTaskStatus.NEW,
> + BugTaskStatus.INCOMPLETE,
> + BugTaskStatus.CONFIRMED,
> + BugTaskStatus.TRIAGED,
> + BugTaskStatus.INPROGRESS,
> + BugTaskStatus.FIXCOMMITTED,
> + BugTaskStatus.FIXRELEASED))

You can outdent all of this by four spaces; we use four-space indents
per level in method calls.

> +
> + sub_bugtasks = []
> + sub_bugs = []
> +
> + for task in bugtasks:
> + if task.bug not in sub_bugs:
> + sub_bugtasks.append(task)
> + sub_bugs.append(task.bug)

Is this loop going to cause performance problems for people with a large
number of direct subscriptions?

> + return BatchNavigator(sub_bugtasks, self.request)
> +
> + def getSubscriptionsPageHeading(self):
> + """The header for the subscriptions page."""
> + return "Subscriptions for %s" % self.context.displayname
> +
> + @property
> + def label(self):
> + return self.getSubscriptionsPageHeading()
> +
> +
> class PersonVouchersView(LaunchpadFormView):
> """Form for displaying and redeeming commercial subscription vouchers."""
>
> === added file 'lib/lp/registry/stories/person/xx-person-subscriptions.txt'
> --- lib/lp/registry/stories/person/xx-person-subscriptions.txt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/stories/person/xx-person-subscriptions.txt 2010-09-10 22:30:12 +0000
> @@ -0,0 +1,56 @@
> +Subscriptions View
> +==================
> +
> +Direct bug subscriptions
> +
> +Each person and team has a subscriptions page that lists things to which they
> +are directly subscribed.
> +
> + >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/+subscriptions')
> + >>> print anon_browser.title
> + Subscriptions : “Ubuntu Team” team
> +
> +The bug subscriptions table does not appear for people without any direct bug
> +subscriptions.
> +
> + >>> page_text = extract_text(find_main_content(anon_browser.contents))
> + >>> "does not have any direct bug subscriptions" in page_text
> + True
> +
> +The bug subscriptions table does appear for someone with direct bug
> +subscriptions and includes the bug number, title and location.
> +
> + >>> anon_browser.open('http://launchpad.dev/~name12/+subscriptions')
> + >>> bug_sub_table = find_tag_by_id(
> + ... anon_browser.contents, 'bug_subscriptions')
> + >>> for tr in bug_sub_table.findAll('tr'):
> + ... print extract_text(tr)
> + Summary
> + In
> + 13
> + Launchpad CSS and JS is not testible
> + Launchpad
> + 5
> + Firefox install instructions should be complete
> + mozilla-firefox (Ubuntu Warty)
> + 4
> + Reflow problems with complex page layouts
> + Mozilla Firefox
> + 2
> + Blackhole Trash folder
> + mozilla-firefox (Debian Woody)
> + 9
> + Thunderbird crashes
> + thunderbird (Ubuntu)
> +
> +
> +The bug subscriptions table also includes an unsubscribe link for bugs to
> +which the person or team is subscribed.
> +
> + >>> browser = setupBrowser(auth='Basic <email address hidden>:test')

You can use user_browser here instead of creating your own.

> + >>> browser.open('http://launchpad.dev/~name12/+subscriptions')
> + >>> unsub_link = browser.getLink(
> + ... id='unsubscribe-subscriber-12')
> + >>> unsub_link.click()
> + >>> print browser.url
> + http://bugs.launchpad.dev/launchpad/+bug/12/+subscribe