Hi Bryce, This is good stuff, but needs some more work before it can land. From a code point of view I think the most important points are: [3] Nothing is returned from a property getter, though I think the test for this is correct. I suspect the test hasn't been run ;) [8] Interface for Reference field needs updating. This is preventing the branch from building. Also, a simple test for accessing the field via the web API should be added, if one does not yet exist. Gavin. [1] I think you'll need to get a database review for database/schema/comments.sql, database/schema/patch-2208-19-0.sql and database/schema/patch-2208-20-0.sql. I think stub and jml are the only ones who can grant that. [2] + distro_source_package = exported( I was suprised to find that this spelling is more common than distribution_source_package. No change needed here, just an observation. [3] + def _getDistroSourcePackage(self): + """Retrieves the corresponding source package""" + if self.distribution is None or self.source_package_name is None: + return None + self.distribution.getSourcePackage( + self.source_package_name) This doesn't return the distribution source package, so I'm not sure how test_link_distro_source_package succeeds! Also, as it's intended solely for a property, I think it should be named after the property, thus _get_distro_source_package(). I think it's clearer if the method naming policy is bent a little here. [4] + def _setDistroSourcePackage(self, distro_source_package): Same naming suggestion as in [3]. It doesn't matter much though. [5] + distro_source_package = property(_getDistroSourcePackage, + _setDistroSourcePackage) Consider adding a doc parameter, even if it just says "See `IBugTrackerComponent`". [6] {{{ ./lib/lp/bugs/interfaces/bugtracker.py 524: E501 line too long (82 characters) 524: Line exceeds 78 characters. ./lib/lp/bugs/model/bugtracker.py 92: 'SourcePackageName' imported but unused 89: 'IDistributionSourcePackage' imported but unused 770: E501 line too long (80 characters) 775: E302 expected 2 blank lines, found 1 770: Line exceeds 78 characters. ./lib/lp/bugs/tests/test_bugtracker_components.py 16: E302 expected 2 blank lines, found 1 }}} Please fix this lint. [7] make lint also makes the following complaints: {{{ database/sampledata/current.sql database/sampledata/lintdata.sql differs from database/sampledata/current.sql. Patches to the schema, or manual edits to database/sampledata/current.sql do not match the dump of the launchpad_ftest_template database. If database/sampledata/lintdata.sql is correct, copy it to database/sampledata/current.sql. Otherwise update database/sampledata/current.sql and run: make schema make newsampledata cd database/sampledata cp newsampledata.sql database/sampledata/current.sql Run make schema again to update the test/dev database. database/sampledata/current.sql database/sampledata/lintdata-dev.sql differs from database/sampledata/current-dev.sql. Patches to the schema, or manual edits to database/sampledata/current-dev.sql do not match the dump of the launchpad_dev_template database. If database/sampledata/lintdata-dev.sql is correct, copy it to database/sampledata/current-dev.sql. Otherwise update database/sampledata/current-dev.sql and run: make schema make newsampledata cd database/sampledata cp newsampledata-dev.sql database/sampledata/current-dev.sql Run make schema again to update the test/dev database. }}} Please look into these. [8] + distro_source_package = exported( + Reference( + Interface, This interface should be IDistributionSourcePackage, shouldn't it? I think this is preventing the branch from building. Are there any tests for using this attribute via the web API already in the tree? If not, I think it's worth adding something simple. [9] + self.assertTrue(component.distro_source_package is None) ... + self.assertTrue(component.distro_source_package is not None) If written as: self.assertIs(None, component.distro_source_package) ... self.assertIsNot(None, component.distro_source_package) these assertions would provide more useful information on failure.