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.