Reviewed code should pass all of this checklist. This is intended as a summary of the policy extensively described in the [[LaunchpadHackingFAQ]], [[https://launchpad.readthedocs.io/en/latest/guides/python.html|Python style guide]], TestsStyleGuide. == Configuration == * Will this configuration change affect production machines? * Do you need to talk to IS 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? == Style == * Is the code layout [[https://www.python.org/dev/peps/pep-0008/|PEP8]] compliant? * Is the format of multi-line docstrings [[https://www.python.org/dev/peps/pep-0257/|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? == User interface == * 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? * Does Java``Script code conform to our JavaScriptStyleGuide? * If Java``Script has been added or changed, is there a useful fallback when Java``Script is not available? * If the code adds/changes Java``Script or CSS, has it been verified in multiple browsers? * 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 design review? == Content-classes, interfaces, and SQL == * Does each database table map to a content class in `lib/lp/*/model`? 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` (or `enums.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 as defined in LaunchpadSecurityPolicy? * Are there tests for API changes or additions in the content class? * Does it use SQLObject or raw SQL when it could use Storm instead? * Are multiline SQL triple-quoted? * If the code is SQL fetching a single object or None, does it use a `selectOne*()` (SQLObject) or `.one()` (Storm) variant? * When using raw SQL, are `quote()` and `sqlvalues()` used to interpolate data safely? * 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) == View Classes == * Are values offered using properties and attributes, rather than no-arg methods? == Gotchas == * Use of the built-in `hasattr` function should be avoided since it swallows exceptions. Use the `safe_hasattr()` function from `lazr.restful`: {{{ from lazr.restful.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? == Tests == * Do the doctests added to `lib/lp/*/doc` read 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 `lazr.config` modifications, calls to `socket.setdefaulttimeout()`) done by the test reverted once the test completed (successfully or not)? * If the test fails, will the failure message be helpful in diagnosing the problem (e.g. the test avoids 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)?