FINERACT-2383: Modernize and correct README page

Author: steinwinde

CONTRIBUTING.md

@@ -1,16 +1,105 @@
+# Contributing
+
 Hello there!
 
-First of all, **Thank You** for contributing to Apache Fineract, we're grateful for your interest in our project.
+First of all, **Thank You** for contributing to Apache Fineract! We are grateful for your interest in this project.
+
+Please join the [developer mailing list](https://fineract.apache.org/#contribute), if you have not already done so, as this is where discussions about this project take place - say _Hi_ there! Please also have a quick look at the [Code of Conduct](CODE_OF_CONDUCT.md).
+
+The [JIRA Dashboard](https://issues.apache.org/jira/secure/Dashboard.jspa?selectPageId=12335824) shows what's going on. Create a login - it can take a day or two. If you face difficulties, ask on the mailing list.
+
+You don't need to be a committer to provide pull requests, but [Becoming a Committer](https://cwiki.apache.org/confluence/display/FINERACT/Becoming+a+Committer) explains the process of becoming one - just in case...
+
+
+## How We Code
+
+### Checkstyle and Spotless
+
+This project enforces its code conventions using [checkstyle.xml](config/checkstyle/checkstyle.xml) through Checkstyle and [fineractdev-formatter.xml](config/fineractdev-formatter.xml) through Spotless. They are configured to run automatically during the normal Gradle build, and fail if there are any violations detected. You can run the following command to automatically fix spotless violations:
+```bash
+./gradlew spotlessApply
+```
+Since some checks are present in both Checkstyle and Spotless, the same command can help you fix some of the Checkstyle violations too.
+
+You can also check solely for Spotless violations, but normally don't have to, because regular builds already include this:
+```bash
+./gradlew spotlessCheck
+```
+
+We recommend that you configure your favourite Java IDE to match those conventions. For Eclipse, you can go to
+Window > Java > Code Style and import the aforementioned [config/fineractdev-formatter.xml](config/fineractdev-formatter.xml) under formatter section and [config/fineractdev-cleanup.xml](config/fineractdev-cleanup.xml) under cleanup section. 
+
+You could also use Checkstyle directly in your IDE, but you don't have to: it may just be more convenient for you.  For Eclipse, use https://checkstyle.org/eclipse-cs/ and load our checkstyle.xml into it. For IntelliJ you can use [CheckStyle-IDEA](https://plugins.jetbrains.com/plugin/1065-checkstyle-idea).
+
+
+### Code Coverage
+
+Changed or added code should ideally have test coverage.
+
+The project uses Jacoco to measure unit tests code coverage. To generate a report run the following command:
+```bash
+./gradlew clean build jacocoTestReport
+```
+Generated reports can be found in the build/code-coverage directory.
+
+
+### Error Handling
+
+* When catching exceptions, either rethrow them, or log them.  Either way, include the root cause by using `catch (SomeException e)` and then either `throw AnotherException("..details..", e)` or `LOG.error("...context...", e)`.
+* Completely empty catch blocks are VERY suspicious.  Are you sure that you want to just "swallow" an exception?  Really, 100% totally absolutely sure?? ;-) Such "normal exceptions which just happen sometimes but are actually not really errors" are almost always a bad idea, can be a performance issue, and typically are an indication of another problem - e.g. the use of a wrong API which throws an Exception for an expected condition, when really you would want to use another API that instead returns something empty or optional.
+* In tests, you'll typically never catch exceptions, but just propagate them, with `@Test void testXYZ() throws SomeException, AnotherException`..., so that the test fails if the exception happens.  Unless you actually really want to test for the occurrence of a problem - in that case, use [JUnit's Assert.assertThrows()](https://github.com/junit-team/junit4/wiki/Exception-testing) (but not `@Test(expected = SomeException.class)`).
+* Never catch `NullPointerException` & Co.
+
+### Logging
+
+* We use [SLF4J](http://www.slf4j.org) as our logging API.
+* Never, ever, use `System.out` and `System.err` or `printStackTrace()` anywhere, but always `LOG.info()` or `LOG.error()` instead.
+* Use placeholder (`LOG.error("Could not... details: {}", something, exception)`) and never String concatenation (`LOG.error("Could not... details: " + something, exception)`)
+* Which Log Level is appropriate?
+    * `LOG.error()` should be used to inform an "operator" running Fineract who supervises error logs of an unexpected condition.  This includes technical problems with an external "environment" (e.g. can't reach a database), and situations which are likely bugs which need to be fixed in the code.  They do NOT include e.g. validation errors for incoming API requests - that is signaled through the API response - and does (should) not be logged as an error.  (Note that there is no _FATAL_ level in SLF4J; a "FATAL" event should just be logged as an _ERROR_.)
+    * `LOG.warn()` should be using sparingly.  Make up your mind if it's an error (above) - or not!
+    * `LOG.info()` can be used notably for one-time actions taken during start-up.  It should typically NOT be used to print out "regular" application usage information.  The default logging configuration always outputs the application INFO logs, and in production under load, there's really no point to constantly spew out lots of information from frequently traversed paths in the code about what's going on.  (Metrics are a better way.)  `LOG.info()` *can* be used freely in tests though.
+    * `LOG.debug()` can be used anywhere in the code to log things that may be useful during investigations of specific problems.  They are not shown in the default logging configuration, but can be enabled for troubleshooting.  Developers should typically "turn down" most `LOG.info()` which they used while writing a new feature to "follow along what happens during local testing" to `LOG.debug()` for production before we merge their PRs.
+    * `LOG.trace()` is not used in Fineract.
+
+## Change Process
+
+### Dependency Upgrades
+
+This project uses a number of 3rd-party libraries. We have set up [Renovate's bot](https://github.com/renovatebot/github-action) to automatically raise Pull Requests for our review when new dependencies are available.
+
+Our `ClasspathHellDuplicatesCheckRuleTest` detects classes that appear in more than 1 JAR.  If a version bump in [build.gradle](https://github.com/apache/fineract/blob/develop/build.gradle) causes changes in transitives dependencies, then you may have to add related `exclude` to our [dependencies.gradle](https://github.com/apache/fineract/search?q=dependencies.gradle).  Running `./gradlew dependencies` helps to understand what is required.
+
+### Pull Requests
+
+We request that your commit message includes a FINERACT JIRA issue and a one-liner that describes the changes.
+Start with an upper case imperative verb (not past form), and a short but concise clear description. (E.g. "FINERACT-821: Add enforced HideUtilityClassConstructor checkstyle").
+
+If your PR is failing to pass our CI build due to a test failure, then:
+
+1. Understand if the failure is due to your PR or an unrelated unstable test.
+1. If you suspect it is because of a "flaky" test, and not due to a change in your PR, then please do not simply wait for an active maintainer to come and help you, but instead be a proactive contributor to the project - see next steps.  Do understand that we may not review PRs that are not green - it is the contributor's (that's you!) responsibility to get a proposed PR to pass the build, not primarily the maintainers.
+1. Search for the name of the failed test on https://issues.apache.org/jira/, e.g. for `AccountingScenarioIntegrationTest` you would find [FINERACT-899](https://issues.apache.org/jira/browse/FINERACT-899).
+1. If you happen to read in such bug reports that tests were just recently fixed, or ignored, then rebase your PR to pick up that change.
+1. If you find previous comments "proving" that the same test has arbitrarily failed in at least 3 past PRs, then please do yourself raise a small separate new PR proposing to add an `@Disabled // TODO FINERACT-123` to the respective unstable test (e.g. [#774](https://github.com/apache/fineract/pull/774)) with the commit message mentioning said JIRA, as always.  (Please do NOT just `@Disabled` any existing tests mixed in as part of your larger PR.)
+1. If there is no existing JIRA for the test, then first please evaluate whether the failure couldn't be a (perhaps strange) impact of the change you are proposing after all.  If it's not, then please raise a new JIRA to document the suspected Flaky Test, and link it to [FINERACT-850](https://issues.apache.org/jira/browse/FINERACT-850).  This will allow the next person coming along hitting the same test failure to easily find it, and eventually propose to ignore the unstable test.
+1. Then (only) Close and Reopen your PR, which will cause a new build, to see if it passes.
+1. Of course, we very much appreciate you then jumping onto any such bugs and helping us figure out how to fix all ignored tests!
+
+[Pull Request Size Limit](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit)
+documents that we cannot accept huge "code dump" Pull Requests, with some related suggestions.
+
+Guideline for new Feature commits involving Refactoring: If you are submitting a PR for a new feature,
+and it involves refactoring, try to differentiate "new feature code" from "refactored" by placing
+them in different commits. This helps to review your code faster.
+
+We have an automated bot which marks pull requests as "stale" after a while, and ultimately automatically closes them.
 
-Please do [join our developer mailing list](https://fineract.apache.org/#contribute), as it's where discussions about this project take place - say _Hi_ there!
 
-Our [JIRA Dashboard](https://issues.apache.org/jira/secure/Dashboard.jspa?selectPageId=12335824) is another good way to see what's going on (do create a Login).
+### Merge Strategy
 
-Here are a few additional useful pointers:
+This project's committers typically prefer to bring your pull requests in through _Rebase and Merge_ instead of _Create a Merge Commit_. (If you are unfamiliar with GitHub's UI regarding this, note the somewhat hidden little triangle drop-down at the bottom of the PR, visible only to committers, not contributors.)  This avoids the "merge commits" which we consider to be somewhat "polluting" the project's commit log history view.  We understand this doesn't give an easy automatic reference to the original PR (which GitHub automatically adds to the merge commit message it generates), but we consider this an only very minor inconvenience; it's typically relatively easy to find the original PR even just from the commit message, and JIRA.
 
-* https://github.com/apache/fineract/#pull-requests
-* https://github.com/apache/fineract/#checkstyle-and-spotless
-* https://github.com/apache/fineract/#logging-guidelines
-* https://github.com/apache/fineract/#error-handling-guidelines
+We expect most proposed PRs to typically consist of a single commit.  Committers may use _Squash and merge_ to combine your commits at merge time, and if they do so, will rewrite your commit message as they see fit.
 
-Note also [our Code of Conduct](CODE_OF_CONDUCT.md).
+Neither of these two are hard absolute rules, but mere conventions.  Multiple commits in single PRs make sense in certain cases (e.g. branch backports).