Launchpad itself

Code review comment for lp:~edwin-grubbs/launchpad/bug-446074-blacklist-form

  1. bug-446074-blacklist-form
  2. Merge into devel
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Edwin.

Thank you very much for providing this fine implementation. There are many
places where I discovered you had implemented the feature as I would hope
(using RegistryEditFormView, and making the table sortable). I think there
is a chance for an oops in the code, and there are some historic code that
I hope you can clean up.

The page titles in the browser are broken, breadcrumbs are missing. This is
because the there is no breadcrumb adapter This might help:

{{{

@adapter(INameBlackListSet)
class NameBlacklistSetBreadcrumb(Breadcrumb):
    """Return a breadcrumb for an `INameBlackListSet`."""
    text = "Name Blacklist"

@adapter(INameBlackList)
class NameBlacklistBreadcrumb(Breadcrumb):
    """Return a breadcrumb for an `INameBlackList`."""

    @property
    def text(self):
        return self.context.regexp

}}}

> === added file 'lib/lp/registry/browser/nameblacklist.py'
> --- lib/lp/registry/browser/nameblacklist.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/browser/nameblacklist.py 2010-11-09 05:26:23 +0000
...
> +class NameBlacklistAddView(NameBlacklistValidationMixin, LaunchpadFormView):
> + """View for adding a blacklist expression."""
> +
> + schema = INameBlacklist
> + field_names = ['regexp', 'comment']
> + label = "Add a new blacklist expression"
> +
> + custom_widget('regexp', TextWidget, displayWidth=60)
> +
> + @property
> + def page_title(self):
> + """The page title."""
> + return self.label

This could be
    page_title = label

...

> +class NameBlacklistSetView(LaunchpadView):
> + """View for /+nameblacklists top level collection."""
> +
> + page_title = (
> + 'Blacklist for names of Launchpad pillars, persons, and teams')
> + label = page_title

I think
    Blacklist for names of Launchpad pillars and persons
is good enough. I think user and team are really subclasses of IPerson.
One day we will also have agent for the bots we have impersonating users.

> +class NameBlacklistSetNavigation(Navigation):
> +
> + usedfor = INameBlacklistSet
> +
> + def traverse(self, name):
> + return self.context.get(int(name))

Do I get a 404 or a 500 for
    http://launchpad.net/+nameblacklist/word

A 404 is correct. The interface says None will be returned (404), but int()
will raise an Exception. Maybe int() should be pushed into get() or this
method chooses to raise the 404 itself when the name cannot be cast.

...

> === added file 'lib/lp/registry/model/nameblacklist.py'
> --- lib/lp/registry/model/nameblacklist.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/model/nameblacklist.py 2010-11-09 05:26:23 +0000
...
> + def get(self, id):
> + """See `INameBlacklistSet`."""
> + store = IStore(NameBlacklist)
> + return store.find(NameBlacklist, NameBlacklist.id == id).one()

I think this method could cast the id to an int and return None when an
exception is raised to avoid a query that will always return None.

...

> === added file 'lib/lp/registry/templates/nameblacklists-index.pt'
> --- lib/lp/registry/templates/nameblacklists-index.pt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/templates/nameblacklists-index.pt 2010-11-09 05:26:23 +0000
> @@ -0,0 +1,40 @@
> +<html
> + xmlns="http://www.w3.org/1999/xhtml"
> + xmlns:tal="http://xml.zope.org/namespaces/tal"
> + xmlns:metal="http://xml.zope.org/namespaces/metal"
> + xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> + xml:lang="en"
> + lang="en"
> + dir="ltr"
> + metal:use-macro="view/macro:page/main_side"
> + i18n:domain="launchpad">
> +
> + <body>
> +
> + <tal:side metal:fill-slot="side">
> + <tal:menu replace="structure view/@@+global-actions" />
> + <tal:menu replace="structure context/@@+related-pages" />

I like @@+related-pages in the side bar, but I do not see anything registered
to provide this menu. Is this needed?

...

> === modified file 'lib/lp/registry/tests/test_nameblacklist.py'
> --- lib/lp/registry/tests/test_nameblacklist.py 2010-10-04 19:50:45 +0000
> +++ lib/lp/registry/tests/test_nameblacklist.py 2010-11-09 05:26:23 +0000
...
> @@ -52,21 +64,25 @@
> # A name that is blacklisted returns the id of the row in the
> # NameBlacklist table that matched. Rows are tried in order, and the
> # first match is returned.
> - self.failUnlessEqual(self.name_blacklist_match("foobar"), -200)
> - self.failUnlessEqual(self.name_blacklist_match("barfoo"), -100)
> + self.failUnlessEqual(
> + self.name_blacklist_match("foobar"),
> + self.caret_foo_exp.id)
> + self.failUnlessEqual(
> + self.name_blacklist_match("barfoo"),
> + self.foo_exp.id)

from https://dev.launchpad.net/TestsStyleGuide:
    When asserting for equality use the form assertEqual
Can you update all these old tests to assertEqual?

...

> +class TestNameBlacklistSet(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(TestNameBlacklistSet, self).setUp()
> + registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
> + registry_expert = self.factory.makePerson()
> + login(ADMIN_EMAIL)
> + registry_experts.addMember(registry_expert, registry_expert)
> + login_person(registry_expert)

You can replace the last 5 lines with
        login_celebrity('registry_experts')

> + self.name_blacklist_set = getUtility(INameBlacklistSet)
> +
> + def test_create_with_one_arg(self):
> + # Test NameBlacklistSet.create(regexp).
> + name_blacklist = self.name_blacklist_set.create(u'foo')
> + self.assertTrue(verifyObject(INameBlacklist, name_blacklist))
> + self.assertEquals(u'foo', name_blacklist.regexp)
> + self.assertIs(None, name_blacklist.comment)

Use the singular form of assertEqual().

> + def test_create_with_two_args(self):
> + # Test NameBlacklistSet.create(regexp, comment).
> + name_blacklist = self.name_blacklist_set.create(u'foo', u'bar')
> + self.assertTrue(verifyObject(INameBlacklist, name_blacklist))
> + self.assertEquals(u'foo', name_blacklist.regexp)
> + self.assertEquals(u'bar', name_blacklist.comment)
> +
> + def test_get(self):
> + # Test NameBlacklistSet.get().
> + name_blacklist = self.name_blacklist_set.create(u'foo', u'bar')
> + store = IStore(name_blacklist)
> + store.flush()
> + retrieved = self.name_blacklist_set.get(name_blacklist.id)
> + self.assertEquals(name_blacklist, retrieved)

If you inline the cast to int() we need a variation of this test.

review: Needs Fixing (code)

« Back to merge proposal