During our week in Brno, we discussed what are the things to check during review. I am presenting a proposal for further discussion. The idea is to put this to wiki as a reference so other reviews can make it a comment (reply) template:
Ticket number, title and description correct?
Does it fix the problem described in the issue?
No unrelated changes?
Redmine opened and in correct project?
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?
No exceptions are swaloved/squashed.
Are raised exceptions type of Foreman::Exception or WrappedException?
Unit/functional/integration test(s) present?
New view templates have .html.erb extensions, not just .erb
Mixins use ActiveSupport::Concern instead of class_eval etc.
Concerns and new classes are added to app/ and not lib/
Apipie documentation is correct: HTTP methods, names of parameters, required true/false
Scoped_search definitions are added for new model attributes where appropriate.
Scoped_search definitions for virtual fields (:ext_method) use :only_explicit
Makes use of logging appropriately?
Pass block to logger.debug to lazily evaluate expensive debug statements
Deprecations use Foreman::Deprecation with a deadline of latest stable + three versions.
Not changing structure or behavior of the public HTTP API?
No memory or performance concerns?
Any packaging needed?
User documentation needed?
Demo worthy?
Any other actions need to be taken after merge?
Please discuss (add your own ones) and I am going to put this to our page Reviewing patches - Foreman and a link to this page to our PR template. Thanks all!
In nearly all cases reading through the list, these checks look like they could be automated and extract the human element from them. @ekohl has played around with a new app for setting github statuses. What if we operationalized these checks to handle the scale of PRs and eliminate the human element? The rest can be included as a automated comment either whole hog or conditionally (e.g. if a PR includes .to_sym then “No use of .to_sym, .send or other reflection on untrusted inputs.” is included).
The longer the list is the less likely reviewers will use it when doing reviews, so I would prefer a short(er) list with the most important and common points. I would remove things we already have rubocop or prbot enforcing, and see if there are any others we can start enforcing as well. There are a couple that i don’t recall ever seeing in prs - for example, “New view templates have .html.erb extensions, not just .erb” or “Concerns and new classes are added to app/ and not lib/”. Some of the items could be combined, e.g. "All strings are extracted properly ", scoped search, logging etc. And one final comment, we only do deprecation for 2 releases at most now.
many of them are not something we should request from new/random contributors, but at least the reviewer should open redmine tickets or make sure it will be added somehow
