The policy has long been to seek text reviews, where appropriate. That was not shown here.
|Deletions are marked like this.||Additions are marked like this.|
|Line 52:||Line 52:|
* Are the UI text and/or help changes significant enough to require danhg's review?
- Will this configuration change affect production machines?
- Do you need to talk to the OSAs about this configuration change?
- What does this change assume about the network?
- What network conditions could cause the change to break?
- What host will the change be running on?
Is the code layout PEP8 compliant?
Is the format of multi-line docstrings PEP257 compliant?
Are the properties named_like_properties, the functions named_like_functions and the methods namedLikeMethods?
Do if:/elif: blocks have a matching else: clause?
Does the methods use super() to chain up to their parent?
Are operator.attrgetter and operator.itemgetter used instead of lambda?
- Do all added methods and classes have an appropriate docstring?
- Do all asserts have an appropriate failure message?
Do the templates conform to the UserInterfaceChecklist?
Is the code added to browser modules appropriate, or would it be more reusable and easier to test in the content class?
- Are there tests for new pages, or changes in behaviour in existing pages?
- If the code is changing GET form arguments, will we get OOPSes because of bookmarked URLs?
- Is the user interface text and help text, on pages being changed, up to date with those changes?
- Are the UI text and/or help changes significant enough to require danhg's review?
Content-classes, interfaces, and SQL
Does each database table map to a content class in lib/canonical/launchpad/database? Is the content class in its own file, named by the table (lower case)?
Are all enums in the same interface files as the schemas where they are expressed? E.g. TeamMembershipStatus should be in interfaces/teammembership.py, not in interfaces/person.py.
- Have security.py permissions been set up correctly for new content classes? (maybe security.py and zcml)
Does it use regular permissions like defined in LaunchpadSecurityPolicy
- Are there tests for API changes or additions in the content class?
- Are multiline SQL triple-quoted?
If the code is SQL fetching a single object or None, does it use a selectOne*() variant?
Is quote() and sqlvalues() use to interpolate safely data in the SQL statements?
Is the content-classes code free from references to ILaunchpadBag and check_permission()?
- If the branch descends from a DB patch, new sample data may need to be generated. Specifically, this occurs when a new DB field can be NULL and defaults to NULL, meaning that sampledata doesn't need to be updated to be valid. Sampledata changes must be made before the branch is submitted to PQM.
If the branch contains database classes that uses any non-database attributes, these classes must provide a __storm_invalidated__ method resetting these attributes. (That way it can be reused safely across transactions)
- Are values offered using properties and attributes, rather than no-arg methods?
Use of the built-in hasattr function should be avoided since it swallows exceptions. Use the safe_hasattr() function from lazr:
from canonical.lazr.utils import safe_hasattr
- Does the branch change the assumptions of the code about the network environment it will be running it (does it now connect directly to internet hosts or does it connect to some specific internal server?). Have these changes been run past IS?
Do the doctests added to lib/canonical/launchpad/doc reads well as documentation? Or would they be more appropriate in other non-documentation directories?
- Do the added page tests read well as use-cases?
Is each new interface defined appropriately documented and has a verifyObject() test?
- Can the overhead of the new tests setUp/tearDown fixture be shared with other tests through the use of a layer?
Are all global changes (e.g. globals, sys.path or canonical.config modifications, call to socket.setdefaulttimeout()) done by the test reverted once the test completed (successfully or not)?
- If the test fail, will the failure message be helpful in diagnosing the problem (e.g. the test avoid comparison to True or False when checking for the presence of a string.)?
Do the page tests use the appropriate BeautifulSoup helpers to extract the relevant HTML section for the test?
If the test calls a subprocess that writes to the database, does it call DatabaseLayer.force_dirty_database() to ensure that the testing database gets rebuilt for the next test? (commits in subprocesses are not detected).