ADempiere Best Practices
Outlines the practices to be followed for development within Adempiere.
Overview
This page outlines the practices to be followed for development within Adempiere. The rules and processes described are intended to ensure that the project is able to maintain the high standard of quality that is expected from a business critical application.
Commit Policy
The following guidelines describe the best practice when committing code in the Adempiere repository.
General Principles
We work as a team. A good team working together and sharing ideas will be smarter than any single developer. Communicate your ideas and intent, seek feedback, be kind with criticism, accept criticism with grace, help out and don't hold back.
Use the Pull Request model as much as possible. It provides a means for others to review your code. The Continuous Integration service can test your build prior to merge. Help out by reviewing pull requests submitted by others.
The ability to commit to the Adempiere repository is a privilege, not a right.
To have autonomy you need a high degree of responsibility and authority. You get these by working with the team. There is no place for loners or loose cannons here.
Committers (those that commit directly to the project repository) are expected to take responsibility for their actions and to not use that privilege to bypass the software development process.
If you break something, you should make every effort to fix it yourself, rather than relying on others to clean up for you.
Documentation
All changes to the code base need to be adequately documented so that others can easily find out why a change was made and have the necessary information to allow them to assist with maintaining the new code. The level and amount of documentation should be in proportion to the degree the proposed change will impact on the behavior of Adempiere.
All pull requests/commits must have some associated documentation such as a pull request, bug report or feature request. At a minimum, an issue should be opened on Github and referenced in the commit. The issue should be descriptive. A one-line description is not sufficient to allow others to review your work in a meaningful way.
Issues or bugs reported should include a description of how to reproduce the issue in the GardenWorld demo or a System account. If it's not easily reproducible in GardenWorld, explain in detail what the issue is, in which scenarios the issue appears and the proposed solution (if any).
If you are reporting a feature request also provide a suggested use case. When you have implemented the feature request you must then explain how to use the new functionality.
Larger changes are better documented in a a wiki page that also provides end user guidance.
It is not acceptable to leave it up to others to "read the code" to work out how your contribution works. [3]
Technical Documentation. Ideally the committer should endeavour to provide some technical documentation. This ensures that if the original committer is unavailable, others will be able to maintain the code without resorting to reverse engineering.
Approval
Bug fixing can be done directly. If you are confident that you have found a bug and that you have a fix that does not have any adverse side effects, it is acceptable to commit directly without waiting for community consultation.
However, if there is any doubt, committers are advised to request further feedback before proceeding. [4]
Feature requests require community approval. To ensure that all new features will be useful to the community at large and properly implemented a process of community consultation must be undertaken.
This involves proposing the contribution, addressing any concerns raised by community members (paying particular attention to functional specialists), and receiving a positive vote for the request. See the section on voting below.
Getting community approval for implementing a feature request is not sufficient to guarantee acceptance of a contribution. All the other requirements outlined here must also be met.
New features entering trunk should be submitted for review in an separate branch or through a patch
Community approval may be withheld until others are able to review the actual code, either by the code being committed to a "sandbox" branch, or submitted as a patch.
Other committers may vote against a proposed feature until such a submission is provided for them, if they do so, they should make every effort to review the submission as quickly as possible.[5]
Implementation
The implementation in code of a bug fix or feature contribution must adhere to the following guidelines.
To integrate into trunk a contribution must be complete, documented and testable
Do not commit half finished code into trunk in the hope that someone else will take it over.
If you wish to contribute work that you are unable to complete, supply it as a patch or separate branch. [6]
Don't drop existing elements or functions.
Don't drop "not-added-by-you" things (db elements or functionalities). We must assume that any existing part of Adempiere may be being used by someone's implementation.
Unless there is a good reason to drop it, for example functionality is really broken, do not drop it.
If there is a valid reason then please first ask in forums before considering the drop. [7]
Always review collateral effects of your changes
Every time you touch one line of code or application dictionary please review the dependencies (collaterals). Eclipse helps a lot pointing to the name of the method/variable + right click + references -> workspace. For other code (like callouts, processes, etc) sometimes you need to check the dictionary to see where is it called (I normally review Adempiere_pg.dmp).[8]
Support both Java 5 and Java 6
We officially support both these versions of Java. If you are coding against Java 6 please ensure that code is backwards compatible with 5. [11]
Only use the current trunk version of ModelGenerator
There must be just one official version of ModelGenerator. Private versions of ModelGenerator must be avoided. [12]
Code Style
Code must be properly indented
But don't re-indent fully working code just because it doesn't look exactly right in your IDE. The code might be indented nicely in someone elses IDE and if we keep reformatting according to our particular IDE we'll end up with a lot of commits but nothing really done. We won't be able to track "real" changes. [13]
Proper usage of variable names [14]
All comments and variables in english [15]
No duplicated code [16]
Use constants or MSysConfig instead of hardcoded things [17]
Committing
Follow the best practices for using SVN when committing. [18]
All commits should be atomic:
that is they are the complete code patch that addresses a Bug Report [BR] or Feature Request [FR] in sourceforge.
If the contribution is unusually large it may be committed in small parts but then commit should be distinct functional parts.
Preferably a commit must solve one and only one tracker - this is to ease integration with other branches or maintained versions.
Commit Early in YOUR day:
Only commit if you are around to support or even revert in case of a problems.
Update before commit:
Do an SVN update and ensure all still compiles locally before you commit.
Reference the BR/FR in the commit comments:
include the full url to the sourceforge BR/FR, but also include a short, but descriptive, comment that does not require the reader to go to the BR/FR in sourceforge unless they require details. Example:
[ 2354040 ] Implementation Replication Mode, Type, Eventhttp://sourceforge.net/tracker/index.php?func=detail&aid=2354040&group_id=176962&atid=879335Enhance replication to allow export and import of documents and then execute the document status action.
Synchronise & Build:
after committing sync again with the repository and confirm all still builds without errors.
On successful commit Update the BR/FR:
include the SVN revision reference and set the status & resolution fields as appropriate.
Rules suggested on Nov-2007 to reach TRUNK
http://sourceforge.net/forum/forum.php?thread_id=1881039&forum_id=611167
Proof of stability. This should be base on community feedback on binary release. Optional: Production site, unit and functional test case.
Sample Data. There should be some sample data available.
Maintainer. Must have dedicated maintainer ( individual or company ). Must follow existing Adempiere Developer's criteria.
Branding and copyright issue. No vendor branding, must prove to be original work.
Migration script. Support all supported databases, i.e oracle and postgresql and pl/java
Migration scripts for ADempiere >342 + 353a shouldn't use or implement new pl/java functions.
Migration Process COMPLETE and documented. For new module that replace or enhance existing functionality, it is very important to have the migration process documented.
Voting
For new functionality
Vote takes place in SourceForge forums or trackers
Active members of the community are entitled to vote
A minimum of 72 hours must pass from the call for a vote before the matter can be considered decided
The vote is decided in favour of the simple majority of the participating voters. At least one vote most be received for a vote to count.
A positive vote only indicates that the proposed new functionality is in principle suitable for inclusion in Adempiere. It does not override a fact that a particular implementation of that functionality must also meet all the other requirements listed above.
Suggestions about hiring sponsored developments to ease inclusion in trunk
http://sourceforge.net/forum/forum.php?thread_id=1785728&forum_id=611167
Sponsored development must be released under "GPLv2 or any later version"
If some development is put in wiki, discussed with community, enhanced with community ideas, etc.
The best practice can be to release it with the same Adempiere license. Very possibly community would not help or opine in a sponsored development that will be released as proprietary.
Contributor Agreement
All sponsored development must be contributed signing an Adempiere Contributor Agreement (just one agreement by developer) like the one proposed here: Contributor_Agreement
Initially contributor agreements must be sent to a selected trustee.
This is helping to ensure the first clause. It's a common practice in Open Source projects after the SCO vs IBM case.
Credit for the Author
It is especially important in an open-source project to ensure that the developers and authors of the code are given proper recognition for their work. The credit should include sufficient detail that future developers working on that code can find information about the rational for the design and a point of contact for additional information.
As a minimum, each commit/patch should include modifications to file header that provide the credit for the author. The credit should include references to additional information in the trackers, feature requests and/or patches by both reference number and a url.
Developers should add this information when submitting patches or code for commit. This will make it easier for others to commit the work on the developer's behalf.
Commiters should ensure that the information is included in the patches or code and, if not, add it. Commiters should, where practical, also include the author's name in the commit comment. The committer's name should appear as the "author" (-u option in Mercurial) of the commit. [19]
Peer review
Sponsored development must follow peer review from a committer to be included in trunk
if the developer is a committer, then must have peer review from other committer (same as all contributions in trunk)
Tools used
In order to ease inclusion in core, the development must follow guidelines from the project - about tool used for integration:
SQL migration scripts
if the developer have problems with migration scripts, he must at least provide 2pack package
Must follow Adempiere conventions
Developer must follow Adempiere conventions, i.e. code, directory tree, naming
Adempiere conventions are not clearly written, but Adempiere is big enough to get some conventions just looking what currently we have.
Test Cases
It would be good developer provide/document simple test cases for the contribution
It's a good advice for sponsors when hiring a developer put the condition that deliverables must include documentation and test cases. The developer must know if the money is enough to make such work.
TestFramework Prototype framework for creating user tests
Visibility
Try to keep updated the advance in a wiki page
Simple advice that also can be put as a condition from sponsor to developer in order to maximize the community involvement.
Sponsored developments must be made in a branch and updated frequently
About branches
Branches are allowed (and in fact they're encouraged) to conduct experiments, to work in specific projects or in tasks, or to coordinate team work.
Private branches are allowed - owner(s) of branch rule the branch (all these rules are suggested in this case, but the owner(s) can expand or shrink these rules)
About releases
Official releases must come from trunk
Official release must be tagged as alpha, beta, release candidate or stable.
This is subject to the number of errors open, the completeness of functionalities, and other issues related to quality of release.
Unofficial releases are allowed from branches
in this case the release must have the name of the branch
in this case the release must NOT be tagged as alpha, beta, release candidate or stable, unless community votes and after a review of open bugs, completeness of functionalities, and other issues related to quality of release.
Suggested policies for extensions
Meeting 2008-11-05
According to suggestions received on meeting nov-5-2008 convoked by the more boring ruler.
Generate ID's from centralized dict
Assign specific entity type and prefix rules
It's suggested also to follow entity type and prefix rules for all columns and tables created.
Build on top and not changing kernel
Sometime an extension might replace some code in core but that is usually just a problem in the core rather than changing the definition of an extension
Definition of kernel: common functionalities, like PO, like window management, etc - none related to business rules ...
Work in a branch
It's encouraged to work in an adempiere branch and open for adempiere committers. But anyways it can be analyzed if extension developer wants to open an own project in sourceforge (or any open source hosting site).
You don't need permissions to experiment in your own branch/project - and add things to core. If you want something integrated in trunk you must follow rules stated in this document.
Extension developers must have a playground where they can develop without asking too much about permissions - following some recommendations to allow easy integration.
If several extensions are in competence - is suggested to let them compete as extensions, not including any on trunk
Integrating several extensions at the same time can lead to column collision problems.
Follow the recommended extension architecture
Write Callouts
Write ModelValidator
Don't generate model for your columns (but you can for your new tables), instead of this use the generic getters and setters available in PO.java
License
License must be "GPLv2 or any later version". An explanation can be found at http://www.gnu.org/licenses/gpl-2.0.html (term 9).
No hiding sources
It's encouraged to avoid the SaaS loophole to hide sources (or any other loophole within this license).
Code must be open for everybody in any release.
Certification
If you follow the suggested policies here - you can be certified as adempiere friendly extension - and we can consider including it within the official release.
Coding Standards
Suggestion & Questions Coding Standards
Coding Style
Do not use the conversion from Integer to int, Double to double and so on, because from java5 is doing autoboxing.
Allways use bracelets {}. It is improving code readability and can evict hard to detect bugs.
Don't use transaction names if is not necessary. Better put null.
Known issues:
At present eclipse formatter is not supporting fluent interfaces (see eclipse bug #196001)
How to change parameters of existing method from ADempiere core
ALWAYS keep the old method for backward compatibility and add the @deprecated tag in the javadoc of the method @deprecated since 3.5.3a. Please use {@link #myMethod(...)} instead
Search for all places from ADempiere core where the old method is using and replace with the new one. In this way you will leave the old method with no calls in core and ready to be dropped after some releases.
How to use Adempiere Query class?
How to return only ONE persistent object?
Use the interface to put the Table Name I_C_ValidCombination.Table_Name
Use the StringBuilder when the Where Clause is built
Use the final String whereClause when the Where Clause is not built If you know that your query should return ONLY one result, then you can assert this, and use firstOnly method instead of first method:
How to return ARRAY of objects?
How to return ARRAY of objects and process elements?
How to return one member of an object?
How to pass Timestamp parameter?
How to use Query class with complex where clause: EXISTS?
How to use Adempiere Callout class?
For new callout code i always recomend to use GridTabWrapper class that is wrapping an GridTab object to an "persistent interface". For more info, there is an example on javadoc of the class. I am copy-paste here too:
Benefits:
cleaner code
write algorithms once (and not 1 method for PO and other for GridTab)
eliminate error prone strings
How to use PreparedStatement/ResultSet ?
If you really need to use JDBC queries, then here is a template for that:
Always use DB.getSQLValue*Ex
DB.getSQLValue*Ex methods which have same functionality as their counterpart but in case of an SQLException(checked) then a DBException(unchecked) will be thrown.
This will help us to assure data integrity and to distinguish between exceptions and null values. See: FR 2448461 - Introduce DB.getSQLValue*Ex methods
Always use Trx.run methods
If you want to run a fragment of code that is changing the database data, and if something if failing you need to rollback entire transaction or to rollback to a savepoint, then Trx.run methods are the best option:
Trx.run(TrxRunnable r) - creates a new transaction, runs the runnable object and if something fails then rollbacks the transaction and throw AdempiereException (extends RuntimeException). If all is ok, the transaction is commited.
Trx.run(String trxName, TrxRunnable r) - similar with previous run method, but instead of creating a new transaction it is creating a new savepoint.
References
Sugesstion & Questions Coding Style
When on a single thread class, StringBuilder should be used for String concatenation. If the class is not single threaded, then, StringBuffer should be used for String concatenation.
Testing Policy
Submit your changes to local testing or a peer review before committing unless you are approved by 1st level committer
Publish your test results pls edit according to SF link findings above)
Test Units
The use of FitnesseSlim is thoroughly explored in Cost Engine/Testing with a complete case contribution.
Documentation Policy
Document your sourcecode changes in this wiki under appropriate topic.
Solicit help from others if you cannot document well. Or just start a stub and allow others to expand it.
Intentionally hiding information may get your contribution categorised as proprietary and not fit for admission into trunk.
New features, windows or fields need help text at least in english.
Communication
Bringing ideas (or collecting votes) for a development or bug fix
open a discussion forum and try to keep the discussion in forums (it can be in a tracker, but we have noticed that forums get much more attention than trackers)
after the discussion ends in some proposal then open a tracker - and add a link in the comment to the forum THREAD (to the thread, not to a single message)
after tracker is opened try to keep the discussion on the tracker - unfortunately we cannot close the forum thread (still, maybe in phpbb we can)
When committing
Try to keep all communication related to one theme in one single thread
Every commit must have a related tracker previously opened
It's recommended the commit message have a link to the tracker and a brief explanation of what was done
After the commit add a comment in tracker pointing to the review, it's recommended to add a link to the svn log
To make comments about code please use the tracker - this is to keep ALL information related in just one site
Please avoid writing to the cvslog maillist - writing comments there is spreading related information, so people trying to follow history of an issue must review forums, then trackers, then maillist. This policy is trying to keep all information related and linked properly.
Lawful Vote and Review of Best Practices
For more info please see Project_Charter#Political.
Review is now ongoing until 31st December 2008, where it is accepted and voted en bloc by the whole community in January 2009.
It has to be a full turn-out consensus vote first time round from the active common members in order for it to carry the weight of enforceable law.
Subsequent reviews shall be periodic minimally each quarter and not amended adhoc prior to review date.
Next review date should be not earlier than March 2009.
Mentor Policy
All committers should also seek a mentor or coach among the senior committers already mentioned in the Commit Committee.
Mentees (who are under Mentors) have to assist in providing review and other administrative assistance (this is due to resources constraints at the moment to manage the team).
Adempiere Official Domains and Subdomains
Fair use
Some policies were proposed on this thread:
Money collected from these sites (i.e. advertising) must go to German Foundation and help to sustain the project
Site sponsor, site seeder, and site maintainer can be advertised within a sponsors page and with a little box at the end of each page
If there are zkwebui interface then some advertising links are allowed in the initial page
Collecting visitor statistics must be made public for all community, or at least to project admins on request
The sites must not collect user information - ask for registration - or anything about (unless by the nature of the site it's needed)
No forums or support must happen on these sites - all support must be redirected to sourceforge forums
No wiki must be set up on these sites - all documentation must be redirected to adempiere wiki
The maintainer must be active, it the website becomes outdated more than one month then we must consider dropping the site, or calling for a new maintainer.
Adempiere citizens can call for vote on closing a site if there are at least 3 persons that think it's being abused or not following fair use practices.
Name of the subdomain must be previously discussed with community to reflect the goal and status of the site
Citizens can ask to add or cut advertising on the subdomain sites (via voting process)
Categories: Community | Documentation | Code snippets
main links
documentation
project links
wiki help
search
toolbox
Source
Adapted from http://wiki.adempiere.net/ADempiere_Best_Practices by Michael McKay.
Original contributors:
Teo Sarca
Cristina Ghita Metas
Trifon D3Soft
Victor Perez e-Evolution
Dominik Zajac BayCIX GmbH
Carlos Ruiz
Paul Bowden
Redhuan D. Oon
Last updated