Reviewing patches¶

Not necessarily a definitive list, but things that come to mind.

- [ ] Does it fix the problem described in the issue?
- [ ] Not fixing additional problems not described in the issue.
- [ ] No use of .to_sym, .send or other reflection on untrusted inputs.
- [ ] Not missing string extractions (use :mark_translated: true).
- [ ] All string extractions follows our rules.
- [ ] Added appropriate permissions for non-admin users?
- [ ] New code hasn't been copy-pasted from elsewhere.
- [ ] Do new fields have appropriate validators?
- [ ] Do validators use Rails 3 syntax?  (`validates <field>, <conditions>`)
- [ ] No exceptions are squashed.
- [ ] Are raised exceptions type of Foreman::Exception?
- [ ] Unit tests present?
- [ ] Functional tests present?
- [ ] New view templates have .html.erb extensions, not just .erb
- [ ] New view templates use `%>` instead of `-%>`
- [ ] Mixins use ActiveSupport::Concern fully, with no class_eval, InstanceMethods etc.
- [ ] Concerns and new classes are added to app/ and not lib/
- [ ] All new APIs, or additional model attributes have apipie documentation and are in API views
- [ ] Apipie documentation is correct: HTTP methods, names of parameters, required true/false
- [ ] Commit message follows the [[Reviewing_patches-commit_message_format]]
- [ ] scoped_search definitions are added for new model attributes where appropriate
- [ ] scoped_search definitions for virtual fields (:ext_method) use :only_explicit
- [ ] API functional tests have a valid test first, followed by invalid tests
- [ ] Pass block to logger.debug to lazily evaluate expensive debug statements
- [ ] New Javascript files are added to config/environments/production.rb if not in app.js
- [ ] No new "stylesheet" tags are added to views, they're already in app.css
- [ ] Deprecations use Foreman::Deprecation with a deadline of latest stable + three
- [ ] Update license file when vendoring 3rd party code/assets etc

More info at: http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches

Remarks¶

Not fixing additional problems not described in the issue.¶

If so, split the PR into additional sub-tasks.

All string extractions follows our rules.¶

See http://projects.theforeman.org/projects/foreman/wiki/Translating#Translating-for-developers

No exceptions are squashed.¶

There are no rescue statements throwing away exceptions, therefore stacktraces.

scoped_search definitions for virtual fields (:ext_method) use :only_explicit¶

It can stop free text search breaking if the field definition added doesn't actually exist - i.e. it's a fake field that uses :ext_method to return results. In this case it should use :only_explicit => true too so it isn't searched in free text searches (see also #5777 for another instance of this).

Deprecation warnings¶

For internal APIs etc that are changing, use Foreman::Deprecation.deprecation_warning to add a warning for plugins that are triggering old codepaths.

We ship deprecated methods for two stable releases, giving plugin authors the chance to ship their plugin with compatibility for a couple of versions before forcing their hand. If our most recent stable release is say, 1.9-stable, then the deprecation first goes into develop which becomes 1.10, still in 1.11 and so the deadline would be 1.12. It would be removed after 1.11-stable's branched. Use the most recent stable number, plus three to work out the deadline.