ADempiere Best Practices
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.
Team
- Teo Sarca [1]
- Cristina Ghita Metas : Libero stabilization
- Trifon D3Soft
- Victor Perez e-Evolution
- Dominik Zajac BayCIX GmbH
- Carlos Ruiz - just read links below\
- Paul Bowden
- Redhuan D. Oon
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.
- 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 are expected to take responsibility for their actions.
- If you break something, you should make every effort to fix it yourself, rather than relying on others to clean up for you. 2
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 of documentation should be in proportion to the degree the proposed change will impact on the behaviour of Adempiere.
- All commits must have an associated Bug Report or Feature Request.
- The absolute minimum requirement for documentation is that a SourceForge tracker is opened and referenced in the commit.
- Trackers must be descriptive. One-line trackers are not sufficient to allow others to complete a meaningful review of your work.
- If you're fixing a bug you should provide steps to reproduce the bug in GardenWorld or System.
- 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
Do not commit a change if you only implemented it for one database i. We officially support multiple databases and all changes should work exactly the same on both every database platform. Do not assume that someone else will port it for you. 9 and 10
Support both Java 5 and Java 6 i. 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 i. 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, Event http://sourceforge.net/tracker/index.php?func=detail&aid=2354040&group_id=176962&atid=879335 Enhance 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 29. Proof of stability. This should be base on community feedback on binary release. Optional: Production site, unit and functional test case. 30. Sample Data. There should be some sample data available. 31. Maintainer. Must have dedicated maintainer ( individual or company ). Must follow existing Adempiere Developer's criteria. 32. Branding and copyright issue. No vendor branding, must prove to be original work. 33. Migration script. Support all supported databases, i.e oracle and postgresql and pl/java 34. Migration scripts for ADempiere >342 + 353a shouldn't use or implement new pl/java functions. 35. 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
- See also Sponsorship rules
Advices suggested on Jul-2007
License GPLv2
- 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
/******************************************************************************
* Product: Adempiere ERP & CRM Smart Business Solution *
* Copyright (C) <Company or Author Name> All Rights Reserved. *
* This program is free software; you can redistribute it and/or *
* modify it under the terms of the GNU General Public License *
* as published by the Free Software Foundation; either version 2 *
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful, *
* but WITHOUT ANY WARRANTY; without even the implied warranty of *
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. *
* See the GNU General Public License for more details. *
* You should have received a copy of the GNU General Public License along *
* with this program; if not, write to the Free Software Foundation, Inc., *
* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. *
* *
* @author <autor name>, <autor@email.com>
* BF [ <number> ] or FR [ <number> ] Description about the bug tracker *
* URL of Tracker *
*****************************************************************************/
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
- See File:Libero-formatter.xml .
- 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?
~~~
StringBuilder whereClause = new StringBuilder();
whereClause.append("AD_Client_ID=?"); // #1
whereClause.append(" AND AD_Org_ID=?"); // #2
whereClause.append(" AND C_AcctSchema_ID=?"); // #3
whereClause.append(" AND Account_ID=?"); // #4
MAccount existingAccount = new Query(ctx, I_C_ValidCombination.Table_Name, whereClause.toString(), null)
.setParameters(new Object[]{AD_Client_ID, AD_Org_ID, C_AcctSchema_ID, Account_ID})
.first();
~~~
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:
~~~
count existingAccount = new Query(ctx, I_C_ValidCombination.Table_Name, whereClause, null)
tParameters(new Object[]{AD_Client_ID, AD_Org_ID, C_AcctSchema_ID, Account_ID})
rstOnly();
~~~
How to return ARRAY of objects?
~~~
lic static MAchievement[] getOfMeasure (Properties ctx, int PA_Measure_ID)
{
al String whereClause = COLUMNNAME_PA_Measure_ID+"=?";
t<MAchievement> list = new Query(ctx, I_PA_Achievement.Table_Name, whereClause, null)
tParameters(new Object[]{PA_Measure_ID})
tOrderBy(COLUMNNAME_SeqNo+", "+COLUMNNAME_DateDoc)
st();
urn list.toArray(new MAchievement[list.size()]);
}
~~~
How to return ARRAY of objects and process elements?
~~~
blic static MAchievement[] getOfMeasure (Properties ctx, int PA_Measure_ID)
{
ing whereClause = COLUMNNAME_PA_Measure_ID+"=? AND "+COLUMNNAME_IsAchieved+"=?";
t<MAchievement> list = new Query(ctx, MAchievement.Table_Name, whereClause, null)
tParameters(new Object[]{PA_Measure_ID, true})
tOrderBy(COLUMNNAME_SeqNo+", "+COLUMNNAME_DateDoc)
st();
(MAchievement achievement : list)
{
og.fine(" - " + achievement);
do some processing here
}
urn list.toArray(new MAchievement[list.size()]);
}
~~~
How to return one member of an object?
~~~
lic static int getWindow_ID(String windowName)
{
retValue = 0;
ing whereClause = COLUMNNAME_Name+"=?";
ndow win = new Query(Env.getCtx(), MWindow.Table_Name, whereClause, null)
tParameters(new Object[]{windowName})
rst();
urn = win.getAD_Window_ID();
}
~~~
How to pass Timestamp parameter?
~~~
estamp dateAcct = ...;
ing trxName = ...;
ingBuilder whereClause = new StringBuilder();
reClause.append("C_CashBook_ID=?");
reClause.append(" AND TRUNC(StatementDate)=?");
reClause.append(" AND Processed=?");
//
//
//
sh retValue = new Query(ctx, MCash.Table_Name, whereClause.toString(), trxName)
tParameters(new Object[]{C_CashBook_ID, TimeUtil.getDay(dateAcct), true})
rst()
;
#1
#2
#3
~~~
How to use Query class with complex where clause: EXISTS?
~~~
ingBuilder whereClause = new StringBuilder();
reClause.append("C_Cash.AD_Org_ID=?");
#1
reClause.append(" AND TRUNC(C_Cash.StatementDate)=?");
#2
reClause.append(" AND C_Cash.Processed='N'");
reClause.append(" AND EXISTS (SELECT * FROM C_CashBook cb");
reClause.append(" WHERE C_Cash.C_CashBook_ID=cb.C_CashBook_ID AND cb.AD_Org_ID=C_Cash.AD_Org_ID");
reClause.append(" AND cb.C_Currency_ID=?)");
#3
sh retValue = new Query(ctx, MCash.Table_Name, whereClause, trxName)
tParameters(new Object[]{AD_Org_ID,TimeUtil.getDay(dateAcct),C_Currency_ID})
rst()
;
~~~
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:
~~~
_Asset_Disposed bean = GridTabWrapper.create(mTab, I_A_Asset_Disposed.class);
estamp dateDoc = (Timestamp)value;
n.setDateAcct(dateDoc);
n.setA_Disposed_Date(dateDoc);
~~~
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:
final String sql = "''your SQL SELECT code''"; PreparedStatement pstmt = null; ResultSet rs = null; try { pstmt = DB.prepareStatement(sql, trxName); DB.setParameters(pstmt, new Object[]{...''parameters''...}); rs = pstmt.executeQuery(); while(rs.next()) { ''// process fetched row'' } } // If your method is not throwing Exception or SQLException you need this block to catch SQLException // and convert them to unchecked DBException catch (SQLException e) { throw new DBException(e, sql); } // '''ALWAYS''' close your ResultSet in a finally statement finally { DB.close(rs, pstmt); rs = null; pstmt = null; }
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.
Example 1:
...
/**
* saves the partner, user and employee
*/
private void cmd_save()
{
Trx.run(new TrxRunnable() {
public void run(String trxName) {
MBPartner bp = new MBPartner(getCtx(), bpartner_id, trxName);
bp.setValue(fValue.getText());
bp.setName(fName.getText());
if (bp.get_ID() <= 0) {
bp.setIsEmployee(true);
}
bp.saveEx();
......
}
});
}
Example 2:
...
/**
* get Connection object
*/
private void cmd_save()
{
Trx.run(new TrxRunnable() {
public void run(String trxName) {
try
{
Trx trx = Trx.get(trxName, true);
conn = trx.getConnection();
...
}
catch (SQLException e)
{
throw new AdempiereException(e);
}
}
});
}
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 (http://sourceforge.net/projects/adempiere/forums/forum/610546/topic/3426534):
- 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)