Pull Request Review

There are many pull requests being pushed to the project and you can help the core team by reviewing and testing them.

In this wiki, I want to explain how you can help with this task

Why is this so important?
Pull requests are the default way of contributing code to the project since April 2020. As you know, iDempiere is an open-source software project, therefore, the code is the most important piece of the project. Daily the project is receiving more and more pull requests to address issues or add new features, these bug fixes and features benefit everyone in the community if the code is well written, otherwise, it might harm the whole project if a bad written piece of code gets to the core.

The iDempiere leaders put a lot of emphasis on keeping code of the highest-quality, they are able to do this by reviewing and filtering what gets into the trunk and what needs to be changed before doing so. The problem is that the more requests the project gets, the longer the queue, and there might be requests that get lost in time.

This is where you can help, by reviewing pull requests you can be the first filter if you do it properly you take a lot of work off the leaders' shoulders, helping to get important code into the trunk faster.

How can I review a pull request?

There are many things that need to be reviewed before code gets into the trunk, but it can be roughly summarized in two:

1. Is the code well written and does not add collateral damage or security breaches?
2. Does the code do what it is intended to do?

The first thing that you can do without having to add anything to your local environment is to review the code.

Code Review

• Choose one of the open pull requests.
• Open the list of commits done against the pull request to see the changes in the code that were done by the developer.
• Check that the submitted code complies with the project guidelines to contribute to the trunk. Review that each point in this check list is fulfilled. The main points to consider are:
  • It has to work in PostgreSQL and Oracle.
  • When possible avoid the use of SQL Code, use the data model instead.
  • If changes in the database were made, the migration scripts are present.
  • JDBC should be avoided if it is used because there is no other way, every cursor must be closed in a "finally" block.
  • All the code is written in English.
  • The pull request does not mix purposes (f.i refactoring and new feature) and changes only the necessary classes to address the ticket's goal.

Once you validate that the code is well written and satisfies the project's guidelines and best practices, proceed to pull the code to your local environment to test.

Review Functionality

In your local environment run:

git checkout master
git pull upstream master
git push
git pull --no-commit https://github.com/(pullrequest-github-user)/idempiere.git IDEMPIERE-(Jira-ticket-number) (for example: git pull --no-commit https://github.com/globalqss/idempiere.git IDEMPIERE-1848) 1

1 Sometimes even when you specify --no-commit, git adds the commit to your log. If that is the case, check with git log and run git reset --soft HEAD~ to remove the commit from the log and keep your tree clean.

Once you have the changes in your local environment.

1. Apply the migration scripts if necessary.
2. Run iDempiere and TEST.
3. Test every possible scenario, do not only test what was intended to be solved with the ticket but verify also that previous functionality is not affected by the commit. The system should behave as before except for the new addition.

Revert everything to keep master clean.

After you've performed your review, report your findings on the Jira ticket, or add a comment directly to the pull request.

You can be as specific as possible, describing what you've tested and the results.

If you found any issues, comment on what you've found and how you think it could be solved if you have any suggestions.

If the results are positive, comment that you've tested the ticket and you find the solution to be OK.

TL;DR;

1. Choose one of the open pull requests.
2. Do a code review based on the project's guidelines to contribute to the trunk.
3. Test the ticket's functionality.
4. Make sure there is no collateral damage or security breaches caused by the ticket.
5. Report your findings on JIRA or directly to the pull request.

I recommend you to test thoughtfully and be completely honest about what you did and what you didn't do. This is an open-source community, you have many eyes on you and your reputation will benefit or get hurt depending on what you do.