Check List Peer Review

This page is intended to annotate a check list for peer reviewing pull requests.

The idea is to keep adding things when errors are passed through the review process. In the end, if possible this could be automated using github actions.

Some parts are redacted in first person as is describing actions made by Carlos Ruiz when peer reviewing.

Code Peer Review

These are verifications I normally do over the files in a pull request:

• no files in org.adempiere.ui.zk/theme/default

• if there are changes in org.adempiere.ui.zk/WEB-INF/src/web/theme/default then there must update timestamp in org.adempiere.ui.zk/WEB-INF/src/metainfo/zk/lang-addon.xml

• When modifying a public method, it must be considered if is possible this method is used in plugins (the change will break plugins)
  • If that's the case is preferrable to keep the old method and just add a new one with the changes
  • If necessary mark the old method as deprecated and solve all the warnings

• It is recommended to regenerate serialversionId when:
  • adding or modifying public/protected methods or variables

• Model classes X_ and I_ are required when the migration script:
  • adds a table
  • adds a column
  • adds or modify a reference list value
  • sometimes required when the type of the column changes

• Search for Env.set* Env.get* with direct strings, idea is to use always Env variables (regexp Env.[sg]et.*"[#$])
  • See IDEMPIERE-4251
  • Example, don't use Env.getContext(..."#AD_User_ID"), instead of that you must use Env.getContext(... Env.AD_User_ID)

• Adding CCache to a class must implement ImmutablePOSupport

• do not create weak dependencies, for example querying a specific record by name, or a field that the user can change easily, better use IDs or UUIDs

• Hardcoded IDs or UUIDs must be declared in SystemIDs class

• just use ANSI SQL Syntax
  • better not to use syntax that is specific just for oracle (even if the Convert can work with that)
  • postgresql specific syntax is forbidden in core, in some special cases is admitted with a corresponding if DB.isOracle()...

• Check that the commit doesn't add warnings to eclipse

Migration Scripts

To automate this task I use the scripts compare_scripts.sh and verify_scripts.sh

Verifications for postgresql

• It compares the oracle and postgresql version to be similar
  • At first both scripts are modified to make them more comparable

• Verify if the folder of the migration script matches with the branch
  • scripts for release-8.2 must go to i8.2 folder
  • scripts for master must go to i8.2z folder (this varies after every release)

• Verifies if there are big IDs (IDs with number between 1000000 and 1999999)
  • Normally this verification shows a false positive on AD_Sequence because of the start sequence set to 1000000 by default

• Verifies presence of C_AcctProcessor, AD_Preference, AD_SysConfig in the script
  • Normally operations over these tables must not appear, if they are there it must be reviewed if is valid

• It checks for the string 'U' as a possible wrong entity type assignment
  • Official migration scripts must assign entity type = 'D'

• Verifies presence of "create sequence"
  • Normally there must not be "create sequence" in the migration script, this is caused if you generate the script in a non-vanilla installation using native sequences

• Check register file
  • Verify that there is a line register_migration_script in the script
  • Verifies that the file registered is the same name of the file being verified

Verifications for oracle

• Verifies usage of NVARCHAR2, NCHAR
  • These must not be used, VARCHAR2 and CHAR are accepted

• Verify usage of CHAR in strings with length > 3
  • For strings with length > 3 VARCHAR2 must be used

• Verify usage of VARCHAR2 in strings with length <= 3
  • For strings with length <= 3 CHAR must be used

• Verify usage of VARCHAR2 without the CHAR suffix
  • Every VARCHAR2 defined must have CHAR next to the size to avoid using a BYTE length

• Lines starting with @
  • Lines starting with @ throw errors in sqlplus - they must be fixed

• Too long lines
  • sqlplus errors if a line is longer than 2499 chars SP2-0027

Verifications for both scripts

• BOMB character
  • Sometimes scripts arrive with BOMB character and they error when applied

• Check the format of the timestamp in the script name
  • It must conform to yyyymmddhhmm

• At the end compare both scripts and if there are differences show them in meld for review
  • Pay attention to missing lines, different lines
  • Commonly ALTER TABLE in oracle is shown different because in postgresql is INSERT INTO t_alter_column, in this case verify that columnname and changes are equivalent

• The script must not have direct CREATE INDEX or CREATE VIEW - these must be created in dictionary first and then synchronized

Execution

After the visual peer review is successful, normally I apply the script in postgresql using

bash RUN_SyncDBDev.sh

If there are doubts about any important oracle difference I also start my local oracle instance and apply the script using

bash RUN_SyncDBDev.sh ~/.idempiere/idempieredevora.properties
                      # this is the properties file pointing to my oracle installation