TG Consultancy Checklist

Tr Consultancy Checklist

Abstract
What to do when starting consultancy
What to do while performing consultancy
What to do before creating a pull request
1. Naming conventions
2. Properties
3. Titles
4. Fetchings
5. Unit tests
6. Webui
7. Streaming
8. Security
9. Database changes
10. Retrievers
11. Validators
12. Grouping properties
13. Other
What to do when reviewing a pull request
TODO

Abstract

This document provides some checklists in an environmentally friendly format for preparing and finalising TG-related consultancy activities.

Although some of the entries refer specifically to TTGAMS (TG Air), the document is applicable to all TG-based applications.

What to do when starting consultancy

Update the TG source (if available):
1. $ git pull
2. $ mvn clean
3. $ mvn eclipse:clean
4. $ mvn -DdownloadSources="true" -DdownloadJavadocs="true" eclipse:eclipse
5. $ mvn clean install
6. In Eclipse select all TG modules.
7. Refresh (press F5 or right-click and select Refresh from the popup menu).
8. From the Eclipse menu select Project - Clean, select (or ensure they are selected) the relevant modules, click OK.
9. Any errors at this stage are probably not your fault, and should be brought to the attention of the project (mis-)manager.
Update the tg-libs source (if available):
1. $ git pull
2. $ mvn clean
3. $ mvn eclipse:clean
4. $ mvn -DdownloadSources="true" -DdownloadJavadocs="true" eclipse:eclipse
5. $ mvn clean install
6. In Eclipse select all tg-libs modules.
7. Refresh (press F5 or right-click and select Refresh from the popup menu).
8. From the Eclipse menu select Project - Clean, select (or ensure they are selected) the relevant modules, click OK.
9. Any errors at this stage are probably not your fault, and should be brought to the attention of the project (mis-)manager.
Update TTGAMS or the relevant project to ensure all changes have arrived:
1. $ git pull
2. $ mvn clean
3. $ mvn eclipse:clean
4. $ mvn -DdownloadSources="true" -DdownloadJavadocs="true" eclipse:eclipse
5. In Eclipse select all application modules (e.g. ttgams-dao, ttgams-pojo-bl, ttgams-web-server, ttgams-web-ui).
6. Refresh (press F5 or right-click and select Refresh from the popup menu).
7. From the Eclipse menu select Project - Clean, select (or ensure they are selected) the relevant modules, click OK.
Checkout or create the feature branch in, which you will be working.
Create a database against, which to perform consultancy. This should be your own consultancy database, preferably not used by other consultants to avoid overwriting or altering someone else's carefully constructed data. For some projects (e.g. TTGAMS) a copy of the latest production database is preferred. For other projects (e.g. tgpsa) you can run PopulateDb to create a sample database. You can either use the shared database server at your site, or create a local database with the server software either installed natively on your consultancy workstation, or running in a VM or under Docker (the tgpsa project contains instructions and scripts and scripts to facilitate local Docker database deployment).
(Not TTGAMS) Run PopulateDb for the project in, which you are working. This is to ensure that it is working before you start making changes. Any unexpected errors at this stage are probably not your fault.

Note: Since some time before 16-Jul-2019 PopulateDb in TTGAMS is deprecated (and even broken) in favour of using a copy of the production database.
- If there are any unexpected errors and you have not already done so, try merging changes from the develop (or other appropriate ancestor) branch and repeat these steps from the mvn commands listed above for the feature branch.
- If the unexpected errors persist after merging, they will need to be brought to the attention of the project (mis-)manager, and probably enhanced before further consultancy can take place.
(Not TTGAMS) Run all unit tests for the project in, which you are working. Ideally the unit tests should be run against the same database type as the production deployment (e.g. TTGAMS unit tests should be run against a MSSQL database; tgpsa unit tests should be run against a PostgreSQL database). This is to ensure that they are working before you start making changes. Any unexpected errors at this stage are probably not your fault.
- Note that TTGAMS tests have been recorded as taking about 1.5 hours to run. In general it has been advised to let Jenkins' auto-running of tests when your branch is merged into develop take care of testing, unless you are going to perform changes likely to break a number of tests (or perhaps have a second workstation where tests can be run in parallel).
- If there are any unexpected errors and you have not already done so, try merging changes from the develop (or other appropriate ancestor) branch and repeat these steps from the mvn commands listed above for the feature branch.
- If the unexpected errors persist after merging, they will need to be brought to the attention of the project (mis-)manager, and probably enhanced before further consultancy can take place.
Prepare a medium for taking notes. A physical paper notebook was suggested, but an electronic version is also suitable (and would allow for easier copy/paste).
If schema changes are anticipated (such as adding a new property to an entity, or adjusting an existing property), extract the relevant database schema. This will allow comparing the original schema with a schema extracted once changes are complete in order to craft an SQL script to include with the relevant release. Either use a database tool that can generate schema (such a DBeaver, or perhaps proprietary MSSQL tools) or run class GenDdl to take a snapshot of the schema prior to implementing any changes.
Add the in progress GitHub issue label to the issue being addressed.

What to do while performing consultancy

Do not commit any secrets - password, tokens, certificates, credentials, etc. - especially in .properties files. Once committed, such information becomes part of the repository's history even if it is removed in a subsequent commit. Although most of FMS's repositories are private, tg is not, and in some cases other people may be granted access to private repositories.
Make detailed notes regarding any significant refactoring that is carried out in order to be able to fully justify the refactoring when responding to criticism at a later date.

What to do before creating a pull request

Ensure that all of the following points have been addressed in the changes you are committing:
1. Naming conventions
  1. Ensure that the word "ensure" is not used in unit test names.
  2. Ensure that all field (property) names start with a lowercase character.
  3. Ensure that all method names start with a lowercase character.
  4. Do not use a Header suffix for entity names (e.g. use Invoice instead of InvoiceHeader). Avoid, where possible, other syntactically sugary suffixes like No (e.g. System instead of SystemNo).
  5. Private static final String variable names used as constants for various messages should have uppercase names starting with ERR_ (for errors), WARN_ (for warnings) or INFO_ (for informational/informative messages). An older convention of using MSG_ for informational/informative messages is deprecated, but historical variables named with MSG_ are not required to be changed (see link).
  6. Naming of synthetic entities.
    1. Synthetic entities, which represent separate concepts do not require any special prefixes. This includes reports and entities extending persistent entities. Examples: WorkBoard, PmDashboard, Manpower, InventoryReorder, StockValue (instead of existing ReStockValue), PmRoutineCalendar (instead of existing RePmRoutineCalendar).
    2. Synthetic entities, which extend persistent entities, add additional selection criteria and appear in standalone centres "in place" of persistent entities that they extend, should have prefix Re. Examples: ReWorkActivity, ReWorkOrder, ReDocument, ReGlTransaction.
    3. All other synthetic entities should have prefix Syn. This includes synthetic entities used as properties for special autocompletion logic (e.g. SynSupplierPart and SynReferTo instead of existing ReReferTo) and synthetic entities used in embedded centres (e.g. SynRotableNote, SynLocationSubsystemCertification and SynRotableAndRotablePartAttachment).
  7. Design pattern for Synthetic Entities with multiple sources.
    1. Such entities should have an origin property - refer entity SynLocationSubsystemCertification for inspiration.
    2. Such entities should not attempt to remove duplicates - refer rows 79-82 in entity SynLocationSubsystemCertification for anti-inspiration.
    3. Such entities should allow deletion with a meaningful error - refer SynLocationSubsystemCertificationDao.batchDelete()`` for inspiration, but do not include @SessionRequired`.
    4. Editing should be possible for all origins - refer rows 444-447 in LocationSubsystemWebUiConfig for inspiration.
2. Properties
  1. New String properties should be created including:
```
@IsProperty(length = [positive integer])
@BeforeChange(@Handler(MaxLengthValidator.class))
```
  2. Existing String properties should be updated to include the above length-related annotations, using the current column length in the database as a reference.
  3. When adding a new property, check if it would impact any rendering customisers. If so, enhance the rendering customiser/s.
  4. When deleting a property, include an SQL script to delete any associated records from tables representing entities like WaTypeDefault, WorkActivityStatusRequiredProperty and WorkActivityStatusNotApplicableProperty.
3. Titles
  1. @EntityTitle is necessary in most situations and should include a meaningful description:
```
@EntityTitle(value = "Work Order", desc = "Work Order is usually a job that can be scheduled or assigned to someone.")
public class WorkOrder extends ...
```
  2. @Title is necessary in most situations and should include a meaningful description:
```
@IsProperty
@Title(value = "Quality Inspector", desc = "The person performing quality inspection of the work order.")
private Technician inspector;
```
  3. All entity and most property titles (with the exception of boolean, refer to the point below) should follow capitalisation rules that are normally applied to titles of newspaper articles: http://grammar.yourdictionary.com/capitalization/rules-for-capitalization-in-titles.html
    
    There is even a website, which does this automagically: https://capitalizemytitle.com/
    
    Abbreviations GST, FX, PO, DC, etc. have to be fully capitalised in both entity and property titles.
  4. Titles for boolean properties should avoid excessive capitalisation. Since they contain a question mark at the end, they need to be constructed in such a way that they can be read as a sentence.
```
@IsProperty
@Title(value = "Preferred System only?", desc = "Specifies whether all Systems or only those indicated as Preferred in Rotable Part/System cross-references are used when selecting Rotables based on From/To System.")
private boolean preferredSystemaOnly;
```
    Entities titles contained within such titles (i.e. System) should still be capitalised.
  5. Titles should not include single quotes (') as they do not render as expected in the webui. Unicode character \2019 (right single quotation mark) should be used instead. On macOS this can be typed with Opt-Shift-]. On Windows perhaps try some of these suggestions.
  6. Double quotes should be replaced wtih unicode U+201C (“) and U+201D (”) (left and right, respectively) e.g. desc = "The State where the “From Bulk Store” is located.". Note that while it is possible to include these characters into string constants using their Unicode codes (e.g. desc = "The State where the \u201cFrom Bulk Store\u201d is located."), it is preferred to include them explicitly, as above. On macOS these can be typed simply by pressing Opt-[ (“) and Opt-{ (”). The equivalent single quote values are typed with Opt-] (‘) and Opt-} (’). For those with a "compose" or "AltGr" key on the keyboard, this post provides a few alternatives to directly type them.
4. Fetchings
  1. Check whether any data retrieval needs to be instrumental, or can be uninstrumented (i.e. change co$(Something.class).findByKey(...) to co(Something.class).findByKey() - note the absence of the '$'. See also the page on Fetch Models. Validators and definers should always use uninstrumental versions. Companions should use uninstrumental versions too, unless they intend to modify or save some other entity as part of save().
  2. fetchAll() should generally be avoided, unless you know a reason why it should be used.
5. Unit tests
  1. Ensure that each decision path in the business rules is covered by a separate unit test.
  2. Check that all assertions are appropriate, e.g. assertTrue() is asserting a simple boolean value, assertEquals() is asserting an expected results is the same as an actual result.
  3. Check that arguments to assertEquals() are in the correct order i.e. expected value first, actual value second.
  4. Ensure that BigDecimal values are not compared with assertEquals(). Instead use assertTrue(EntityUtils.equalsEx(...)). This way the scale of values won't affect the comparison result e.g. 0.000 == 0.
  5. Ensure that BigDecimal values, when compared with assertTrue(...) (as above), are constructed with strings representing the value rather than digits e.g. assertTrue(EntityUtils.equalsEx(new BigDecimal("12.34"), actualValue));.
  6. @SuppressWarnings("unused") should be avoided, and all warnings suppressed.
  7. To avoid unit tests undergoing upgrade when run against an MSSQL database in particular (or PostgreSQL, if the application primarily connects to a PostgreSQL database), ensure that all properties that resemble a "transaction date" and are part of a composite key are populated.
  8. Ensure that data population is not being saved to a script, or used from a previously saved script.
  9. Ensure that the constants.setNow(prePopulatedNow); approach is used to set datetime in unit tests.
  10. Endeavour not to populate fields in valuable (unit) test data that are populated as a result of execution of definers. For example if Entity.amount is calculated by a definer as Entity.hours * Entity.rate, then by all means populate Entity.hours and Entity.rate, but do not populate Entity.amount.
6. Webui
  1. When composing a WebUI, ensure that width and minWidth are set in such way that column titles do not appear truncated, unless there are some special circumstances. The browser window should be reduced in size horizontally until the horizontal scroll bar just appears, and the column titles checked.
  2. Ensure that all standard actions are present (e.g. insert, delete, sort, export).
  3. Ensure that the same master is displayed when clicking the insert button as when clicking the edit button for an existing row.
  4. Ensure that desc is on a separate line in masters, especially when using a multi-line editor.
  5. Double-check that small details, like spacing in menu and form titles, have been addressed.
  6. When designing Webui layouts for smaller masters, which do not need to take the most of the screen width, please refrain from specifying width in % and specify in (logical) pixels instead - this will ensure that the desired layout is consistent regardless of the screen resolution and browser zoom.
  7. Ensure that the centre webui layout dimensions are sufficient to display all criteria, and that there are no empty fields with "Element not found!" warnings.
  8. When adding menu items to a compound master, ensure that the Open<class>ActionDao constructor includes a call to addViewBinding() for the new menu item (to ensure that the icons turn blue when the compound master is opened and there is data present).
  9. Ensure that custom SVG icons are optimised to keep the size of the vulcanised application at the absolute minimum. For example, using a service like SVGOMG!.
  10. Do not include unused icons in the iconsets.
  11. Access to entities with only simple masters is still primarily controlled via web menu invisibility.
    
    Note that menu invisibility is per user, not per base user (since the implementation of https://github.com/fieldenms/tg/issues/1734).
    
    If the users who will NOT have access to the menu item are known, list them as shown below. If the users who WILL have access to the menu item are known, make the condition below AND KEY_ NOT IN ('USERNAME1', 'USERNAME2').
    
    For any new menu entries, create an SQL script in TTGAMS/releases with the date and issue number in the filename e.g. 20181101-#1395.sql and add a line like (for MSSQL):
```
INSERT INTO WEBMENUITEMINVISIBILITY_(_ID, _VERSION, OWNER_, MENUITEMURI_, CREATEDDATE_)
   SELECT (NEXT VALUE FOR dbo.TG_ENTITY_ID_SEQ), 0, _ID, 'Users%20%2F%20Personnel/Personnel/Person%20Replacement', GETDATE()
      FROM USER_
      WHERE BASE_ = 'N'
      AND KEY_ IN ('USERNAME1', 'USERNAME2')
```
    or (for PostgreSQL):
```
insert into webmenuiteminvisibility_(_id, _version, owner_, menuitemuri_, createddate_)
   select nextval('tg_entity_id_seq'), 0, _id, 'Users%20%2F%20Personnel/Personnel/Person%20Replacement', current_timestamp
      from user_
      where base_ = 'N'
      and key_ in ('USERNAME1', 'USERNAME2')
```
    The value for MENUITEMURI (sic) is determined from the full menu path (Users / Personnel/Personnel/Person Replacement in this example), with some special characters replaced with their hex ASCII codes.
    
    Note that this populates menu invisibility for the specified users i.e. the named users will not be able to see the menu item.
    
    Another example for TTGAMS/MSSQL, basing a new webui's invisibility on that of an existing webui (rather than based on user), is as follows:
```
INSERT INTO WEBMENUITEMINVISIBILITY_(_ID, _VERSION, OWNER_, MENUITEMURI_, CREATEDBY_, CREATEDDATE_, CREATEDTRANSACTIONGUID_)
  SELECT (NEXT VALUE FOR dbo.TG_ENTITY_ID_SEQ), 0, OWNER_, 'Equipment/Administration/Equipment%20Part%2FInventory%20Part', CREATEDBY_, GETDATE(), CREATEDTRANSACTIONGUID_
    FROM WEBMENUITEMINVISIBILITY_
    WHERE MENUITEMURI_ = 'Equipment/Administration/Rotable%20Part%2FInventory%20Part'
GO
```
  12. The following wiki page may be of some use when testing webui functionality: Window management test plan.
  13. When testing mobile functionality, narrowing the browser window should trigger use of the mobile layout. However in order to test properly, and to ensure that all icons (in the popout sidebar menu of compound masters) are present, select mobile device emulation in a desktop browser (or test on an actual mobile device if available). Testing in both portrait and landscape modes should be considered.
  14. When adding a summary to an EGI (typically with .withSummary()), the artificial property name should not resemble a real property name. For example for a kount of all rows (when using COUNT(SELF)), the artificial property name should be total_count_. For subtotals (SUM()), the property name should be, perhaps, total_time_ instead of timeTotal.
7. Streaming
  1. Ensure that method getAllEntities() is only used when it is certain that a limited number of entities will be returned. Otherwise use streaming (see Streaming Data).
  2. The following anti-pattern should not be used:
```
try (final Stream<Something> stream = coSomething.stream(from(query).lightweight().model())) {
    final List<Something> things = stream.collect(toList());
    if (!things.contains(newThing)) {
        return failure(newThing, ERR_NOT_ACCEPTABLE_THING);
    }
}
```
    Instead, check the existence of newThing during query generation and just check if query count equals 1.
    
    Alternatively use method co.exists(query), for example:
```
final EntityResultQueryModel<Rotable> qRotable = select(Rotable.class).where()
        .prop("rotablePart").eq().val(rotablePart012382).and()
        .prop("serialNo").eq().val(serialNo).model();
assertTrue(co(Rotable.class).exists(qRotable));
```
  3. Most streams, and all streams involving database queries, must be wrapped in try (), for example:
```
try (final Stream<StocktakeDetail> stream = co(StocktakeDetail.class).stream(qem)) {
    stream.forEach(stocktakeDetail -> {
         // do something with stocktakeDetail
    });
}
```
8. Security
  
  Please note that security is not a dirty word.
  
  Now that bulk of the security tokens have been created and mapped, each issue has to take care of its own.
  1. Ensure that you are not committing any secrets - password, tokens, certificates, credentials, etc. - especially in .properties files.
  2. In the dao, ensure that the save() method is overridden and has the appropriate CanSave token, e.g.:
```
@Override
@SessionRequired
@Authorise(GlTransactionType_CanSave_Token.class)
public GlTransactionType save(final GlTransactionType entity) {
    return super.save(entity);
}
```
  3. If deletion is supported, do the same for the batchDelete() method with a CanDelete token, e.g.:
```
@Override
@SessionRequired
@Authorise(Address_CanDelete_Token.class)
public int batchDelete(final Collection<Long> entitiesIds) {
    return defaultBatchDelete(entitiesIds);
}
```
  4. Implement a CanSave token, e.g. GlTransactionType_CanSave_Token.java:
```
package fielden.security.tokens.persistent;

import [...]

public class GlTransactionType_CanSave_Token extends AdministrationModuleToken {
    private final static String ENTITY_TITLE = TitlesDescsGetter.getEntityTitleAndDesc(GlTransactionType.class).getKey();
    public final static String TITLE = Template.SAVE.forTitle().formatted(ENTITY_TITLE);
    public final static String DESC = Template.SAVE.forDesc().formatted(ENTITY_TITLE);
}
```
  5. If deletion is supported, implement a CanDelete token, e.g. Address_CanDelete_Token.java:
```
package fielden.security.tokens.persistent;

import [...]

public class Address_CanDelete_Token extends StoresAndPurchasingModuleToken {
    private final static String ENTITY_TITLE = TitlesDescsGetter.getEntityTitleAndDesc(Address.class).getKey();
    public final static String TITLE = Template.DELETE.forTitle().formatted(ENTITY_TITLE);
    public final static String DESC = Template.DELETE.forDesc().formatted(ENTITY_TITLE);
}
```
  6. Create an SQL script in TTGAMS/releases with the date and issue number in the filename e.g. 20181101-#1395.sql
  7. When migrating security functionality from T64 to TTGAMS, in T64 identify the relevant security string, and then the list of user classes allowed write access to that feature, for example:
```
SELECT * FROM USER_FUNCTIONS
    WHERE TF_REFNO IN (SELECT TF_REFNO FROM TRIDENT_FUNCTIONS WHERE TF_DESC = 'Personnel Utilities') AND
          ALLOW = 'w'
    ORDER BY USER_CODE
```
    This example results in:
```
USER_CODE   TF_REFNO    ALLOW
ADMIN       904         w
SUPER_TI    904         w
SUPERUSE    904         w
TRIDENT     904         w
```
  8. In the newly created SQL script construct an SQL insert statement like this (specifying the new token and the list of user codes identified above) (for MSSQL):
```
INSERT INTO SecurityRoleAssociation_(_ID, _VERSION, SECURITYTOKEN_, ROLE_, CREATEDDATE_)
    SELECT (NEXT VALUE FOR dbo.TG_ENTITY_ID_SEQ), 0, 'fielden.security.tokens.persistent.PersonReplacement_CanSave_Token', _ID, GETDATE()
        FROM USER_ROLE
        WHERE KEY_ in ('ADMIN', 'SUPER_TI', 'SUPERUSE', 'TRIDENT')
```
    or (for PostgreSQL):
```
insert into securityroleassociation_(_id, _version, securitytoken_, role_, createddate_)
    select nextval('tg_entity_id_seq'), 0, 'fielden.security.tokens.persistent.PersonReplacement_CanSave_Token', _id, current_timestamp
        from user_rolE
        where key_ in ('ADMIN', 'SUPER_TI', 'SUPERUSE', 'TRIDENT')
```
9. Database changes
  1. If a new entity is created, or if a new property is added to an existing entity or an existing property is altered, the corresponding database changes must be reflected in an SQL script to be included in the relevant release.
    1. Create an SQL script in TTGAMS/releases with the date and issue number in the filename e.g. 20181101-#1395.sql
    2. Either use a database tool (e.g. DBeaver) or the GenDdl class to extract the relevant schema. GenDdl.java can be modified to connect to the appropriate database by passing a suitable application.properties file to it in the run configuration, and should have the relevant entity added to the .generateDatabaseDdl() call near the bottom of the ghoti. A sample run configuration for Eclipse is as follows (adjust it as required to suit your environment):
      
      And for IntelliJ is as follows (similarly, adjust as required):
    3. Compare the new SQL text with that captured before consultancy commenced, and remove the bits that have not changed. For example if you have added a new property to an existing entity, your final SQL script would simply be an ALTER TABLE x ADD y statement (and whatever else was added with it, such as an index or foreign key).
  2. If activatable dependencies have changed, i.e.:
    - Any properties are annotated with, or no longer annotated with, @SkipActivatableTracking, @SkipEntityExistsValidation(skipActiveOnly = true)
    - Entities are added or removed from @DeactivatableDependencies
    - Properties, which are instances of activatable entities, are added or removed to/from other activatable entities (without any of the above annotations)
    Then an SQL script needs to be included to recalculate refcount.
    
    For example, such an SQL script (for MSSQL; for PostgreSQL it would more closely resemble standard SQL) might resemble this:
```
BEGIN TRANSACTION;

UPDATE Rotable_ SET REFCOUNT_ = 0;

UPDATE Rotable_ SET REFCOUNT_ = REFCOUNT_
  + (SELECT COUNT(*) FROM Equipment_ AS DEPENDENT_
     WHERE DEPENDENT_.ACTIVE_ = 'Y' AND
           DEPENDENT_.Rotable_ = Rotable_._ID
    )
 WHERE ACTIVE_ = 'Y';

COMMIT WORK;
GO
```
    The creation of such SQL scripts can be facilitated by the use of the main() function in ApplicationDomain. The function should be modified to include the class for, which dependencies are sought. Then simply run it. Messages will be displayed if there are no (activatable) dependencies, otherwise a list of such dependencies will be displayed. The list can be considered authoratative as it is generated from the model itself.
10. Retrievers
  1. Any Money properties that are the subject of database migration must be annotated with .amount. For example, properties actualCost and estimatedCost are of type Money and thus annotated with .amount:
```
@Override
public SortedMap<String, String> resultFields() {
    return map(
            field("workOrder.number", "T.WONO"),
            field("person.name", "T.CRAFT"),
            field("actualCost.amount", "COALESCE(T.COST, 0)"),
            field("actualHours", "COALESCE(T.HOURS_ACT, 0)"),
            field("estimatedHours", "COALESCE(T.HOURS, 0)"),
            field("estimatedCost.amount", "COALESCE(T.EST_COST, 0)")
    );
}
```
  2. Entities with optional composite key members must have all members specified in the retriever, with the optional members yielding null where applicable. This is required because of reliance of the cache to match values by indexes of their key members.
  3. Always provide a full field mapping in case of composite entities with a single key member, which effectively replaced String-based keys.
    
    For example, migration of entities with a property of type Vehicle should specify vehicle.number instead of just vehicle (because entity Vehicle has a DynamicEntityKey with one composite key member - number).
    
    For another example, in the above ghoti extract can be seen references to workOrder.number and person.name.
11. Validators
  1. Ensure that you check for nulls in definers and validators that pertain to @CritOnly(Type.SINGLE) properties. Invalid entities cannot be saved, including the cases of required properties having no values. However, entities representing selection criteria can be saved, even if some of the properties that model selection criteria (namely @CritOnly(Type.SINGLE)) are null — this is a feature, not a bug. Hence, when an entity with null values in @CritOnly(Type.SINGLE) properties is loaded, definers for such properties get executed.
  2. Validators should return failure(ERR_MESSAGE); rather than throw failure(ERR_MESSAGE);. This is partly because in general, if there are validators on a persistent entity, throwing a result from a validator would put that entity into an inconsistent state.
  3. Ideally validators will check for error conditions before warning conditions. I.e. any return Result.failure(newValue, ...) should occur before return Result.warning(newValue, ...).
  4. Validators return a Result, such as successful() or warningEx(). These support the passing of an extra parameter - instance, however when returned from a validator parameter instance is not used and should be omitted. Where a Result is returned from a non-validator, instance might be used and in such cases should be included, but otherwise should also be omitted. See here for details.
12. Grouping Properties
  
  Grouping properties are used to group the results of reports and analyses. They are typically implemented as entities with an enum defining a fixed list of possible values, and displayed to the user as selection criteria in the respective centres.
  
  Refer to commits in TG Fleet and TTGAMS for examples.
  1. Such entities should be synthetic i.e. have a field model_ or models_.
  2. They should be annotated with @SupportsEntityExistsValidation.
  3. The related matcher should always return all values.
13. Other
  1. "string".formatted(...) is preferred over format("string", ...).
    
    Beware of using formatted on concatenated strings. For example:
```
final String suggestion = "Hello %s, " + "let's dance %s.".formatted("Friend", "together");
```
    may not provide the expected result, and is better implemented as:
```
final String suggestion = ("Hello %s, " + "let's dance %s.").formatted("Friend", "together");
```
  2. Instead of inheriting a property (e.g. active) from an ancestor class in order to apply a default, the default can be provided in method new_() in the dao.
  3. Instead of using BigDecimal.multiply(new BigDecimal("-1.0")) it is possible to use BigDecimal.negate().
  4. Ensure TODO items that are done have been removed from the ghoti.
  5. Ensure that any System.out.println(); have been removed.
  6. Without attributing any specific ghoti to any specific consultant, the following pattern:
```
Eo eo = null;
if (!locationSubsystemEoList.isEmpty()) {
    eo = locationSubsystemEoList.get(0).getEo();
} else {
    eo = rotableEoList.get(0).getEo();
}
```
    should be replaced by something more like:
```
final Eo eo;
if (!locationSubsystemEoList.isEmpty()) {
    eo = locationSubsystemEoList.get(0).getEo();
} else {
    eo = rotableEoList.get(0).getEo();
}
```
    or even something like:
```
final Eo eo = (!locationSubsystemEoList.isEmpty()) ? locationSubsystemEoList.get(0).getEo() : rotableEoList.get(0).getEo();
```
  7. Ensure that your ghoti complies with the SOLID principle.
  8. Check that entitys are compared using EntityUtils.areEqual() instead of, for example, wa.getWaType().equals(woTypeEO).
  9. Without attributing any specific ghoti to any specific consultant, the following pattern:
```
final WorkActivityStatus workActivityStatusF = fetchWorkActivityStatusUsingWaFetchProvider("F");

assertNotEquals(workActivityStatusF, workActivity189510.getStatus());
```
    could be replaced with:
```
import static fielden.work.common.WorkActivityStatus.FixedStatuses.F;

assertNotEquals(F.name(), workActivity101168.getStatus().getKey());
```
  10. Instead of ghoti like variable.equals(constant) it is safer to use constant.equals(variable).
  11. Ensure that properties of type Money are not annotated with (precision = 18, scale = 2) as this is the default.
  12. Endeavour to not do inside a loop anything that can be done outside a loop, especially database-intensive operations.
  13. Ensure that BigDecimal is never equated to 0. Instead always use comparison with 0 (BigDecimal.ZERO.compareTo(value)).
  14. Particularly in @Calculated expressions, such as ifNull().<some expression>.then().val(Money.zero), use Money.zero (for Money) or new BigDecimal("0.00") for non-Money i.e. do not use BigDecimal.ZERO as this has a precision of 0 and might not produce the intended result.
  15. Methods that are only called from one place do not need to be implemented in the dao - they can be implemented exactly (and only) where they are needed. This is to reduce the scope of the methods. Methods that are called from several different places (e.g. business logic and unit tests) should be implemented in the dao.
  16. Sufficiently trivial methods can be implemented as a default methods in the companion object interface (e.g. IWorkActivity) rather than being declared there and implemented in the dao (e.g. WorkActivityDao). This is to facilitate tracing the ghoti at a later date such as when performing criticism.
  17. It is generally not necessary to explicitly retrieve property.key in a fetch provider as it is implied that the key will be retrieved.
  18. Injecting a companion into validators, definers and companions has apparently been deprecated. Instead the co() API should be used, as sort of documented in the respective TG issue. Note that matchers do not provide this functionality and thus may still use injected companions. Also see this commit.
  19. Joda-Time's DateMidnight is @Deprecated. You are recommended to avoid DateMidnight and use toDateTimeAtStartOfDay() instead. For more information please refer Recommended use for Joda-Time's DateMidnight and Class DateMidnight.
  20. If at all possible, ensure that there are not extra blank lines where they are not required, multiple consecutive blank lines or extraneous trailing spaces. Spaces are preferred over tabs - please ensure that tabs are not inserted and any tabs are converted to spaces.
  21. Without attributing any specific ghoti to any specific consultant, when confronted with the following pattern:
```
final AlternateRotable contraryAlternativeRotable = co(AlternateRotable.class).findByKey(entity.getAlternatePart(), entity.getRotablePart());
if (contraryAlternativeRotable == null) {
    // do the needful
}
```
    it is better to simply do:
```
final boolean contraryExist = co(AlternateRotable.class).entityWithKeyExists(entity.getAlternatePart(), entity.getRotablePart());
if (!contraryExist) {
    // do the needful
}
```
    Note, however, that the default (platform) implementation invokes findByKeyAndFetch() to determine if a suitable instance exists. In some cases findByKeyAndFetch() might be overridden, for example TTGAMS BinDao, to either retrieve and return the existing instance or create a new instance if one does not exist. In those cases entityWithKeyExists() will always return true, which is unlikely to be the desired outcome.
  22. Instead of if (predicate() == false) use if (!predicate()) (and positive equivalent).
  23. Instead of if (expression) return false else return true use return !expression (and positive equivalent).
  24. Instead of coPerson.currentPerson().getUser() use coPerson.getUser().
    
    Or even coPerson.currentPerson().orElseThrow(CURR_PERSON_IS_MISSING).
  25. When enhancing partial query conditions as part of query enhancers and value matchers, exercise caution and care when adding .or() condition. Partial queries constructed outside these enhancers and matchers may already have some conditions constructed using .and() conditions - adding .or() condition is most likely to produce undesired results.
    
    Note that partial queries used as part of user-driven filtering are already enclosed in parentheses in the generated SQL.
  26. Instead of declaring a new ERR_REQUIRED = "Required property [%s] is not specified for entity [%s].", reuse the one now available in MetaProperty i.e. MetaProperty.ERR_REQUIRED.
  27. Ensure that @SessionRequired annotations are not applied to methods, which are private - the methods must be at least protected or public.
  28. Ensure that @SessionRequired annotations are not applied to methods of classes, which do not implement ISessionEnabled interface.
  29. Property bindings that have nothing to do with establishing a database connection should not be put into method mkDbProps() (in H2DomainDrivenTestCaseRunner.java). Instead they should be put into DaoDomainDrivenTestCaseConfiguration.java.
  30. When implementing complex logic as part of EntityDao.save(), especially if it involves modifying other entities, please consider starting with making sure that the entity to be saved is valid before you do anything else:
```
// Make sure that the entity to be saved is valid before we do anything else
entity.isValid().ifFailure(Result::throwRuntime);
```
  31. Unless you really know what you are doing, custom matchers should extend FallbackValueMatcherWith[Centre]Context. Typically, they should be overriding makeSearchCriteriaModel() and ocassinonally other methods. Only in special cases custom matchers should be extending AbstractSearchEntityByKeyWith[Centre]Context - in such cases you should really know why you are doing this and not extending FallbackValueMatcherWith[Centre]Context.
  32. string.replace() is preferred instead of string.replaceAll() where no regular expression is involved - see Micro optimisation: calls to String.replaceAll need to be replaced for details.
  33. Modules.<MODULE>.newException(...) is preferred over new <ModuleException>(...).
  34. CollectionUtil.setOf(...) is preferred over creation of a HashSet as a separate variable.
(If applicable) Run PopulateDb for the project in, which you are working. Ensure it is successful.

Note: Since some time before 16-Jul-2019 PopulateDb in TTGAMS is deprecated (and even broken) in favour of using a copy of the production database.
(If applicable i.e. retriever/s were modified and the project is still in the legacy data migration phase) Run MigrateLegacyDb for the project in, which you are working. Ensure it is successful.

Note: Depending on retriever dependencies, some long-running retrievers can be omitted to optimise the pre-pull request process.
(Not TTGAMS - see note above about TTGAMS tests) Run all unit tests for the project in, which you are working. Ensure they all pass.

At this stage it should be the case that the work you have done is successful in and of itself.
Merge from develop (or from the appropriate ancestor branch).
- Note that it is recommended to recompile (Project - Clean) the project again to ensure that all changes have been recognised and compiled.
(If applicable) Run PopulateDb again. Ensure it is successful.

Note: Since some time before 16-Jul-2019 PopulateDb in TTGAMS is deprecated (and even broken) in favour of using a copy of the production database.
- If it is not successful, it could indicate that some errors were committed into the develop (or ancestor) branch, or that your changes are incompatible with other changes already committed/merged into the develop branch.
- Any errors you are pretty sure you did not cause should be brought to the attention of the project (mis-)manager.
- Any errors you probably did cause will need to be enhanced before starting the commit process again.
(If applicable) Run MigrateLegacyDb for the project in, which you are working. Ensure it is successful.

Note: Depending on retriever dependencies, some long-running retrievers can be omitted to optimise the pre-pull request process.
(Not TTGAMS - see note above about TTGAMS tests) Run all unit tests again. Ensure they all pass.
- If any do not pass, it could indicate that some errors were committed into the develop (or ancestor) branch, or that your changes are incompatible with other changes already committed/merged into the develop branch.
- Any errors you are pretty sure you did not cause should be brought to the attention of the project (mis-)manager.
- Any errors you probably did cause will need to be enhanced before starting the commit process again.
Perform front end testing - ensure it passes.

Note: This is a good opportunity to review the issue requirements and ensure that all have been met.

Note: This is also a good opportunity to capture screenshots of essential parts of the functionality for the preparation of the change overview (TG Air and TG Fleet in particular).
Double-check that all changes have actually been committed and pushed upwind.
For Airways (TG Air) and SAAS (TG Fleet) - write the change overview section of the issue description, including screen captures of the functionality (where relevant) and some notes that will assist both the pull request reviewer and the end user in assessing the functionality.

Note: If the pull request review implements changes during their review that affect the screen captures or the text, it is the responsibility of the pull request reviewer to update the change overview.
Create the pull request, carefully selecting the base branch (usually it will be develop, but perhaps not if your feature branch was created from some other branch). Ensure that the text Resolve #nnn (referencing the original issue) is included in the pull request description. Nominate one or more reviewers for the pull request - those reviewers should receive an email or other notification advising of their nomination, and the pull request should appear in their list of pending pull requests.
Remove the in progress GitHub issue label from the issue that was addressed and add the pull request label.
Send a more direct message to the reviewer/s nominated above advising that their review of the pull request has been requested.

What to do when reviewing a pull request

If any of the steps undergo upgrade, report to the author of the pull request and request rectification.

Update the pull request to set yourself as the reviewer (not the assignee), and add the in progress label.
Update the feature branch from the develop branch (even if there are no conflicts).
Perform, variously, a mvn build/compile/install for all dependencies and affected repositories - ensure everything succeeds.
(Not TTGAMS) Run PopulateDb for the project - ensure it succeeds.

Note: Since some time before 16-Jul-2019 PopulateDb in TTGAMS is deprecated (and even broken) in favour of using a copy of the production database.
If not running PopulateDb (see above), then apply to the appropriate database any SQL scripts implemented as part of the change being reviewed.
(Not TTGAMS - see note above about TTGAMS tests) Run all unit tests for the project (e.g. mvn clean test) - ensure all tests pass.

Note: If specific tests were modified as part of the issue being reviewed, even for TTGAMS, ensure that those tests pass.
Run the application and check, front end, whether it seems that the fundamental requirements in the issue have been addressed.

Note: The change overview section of the issue description may be of assistance for Airways changes.
Run through the What to do before creating a pull request checks and ensure that everything has been attended to.
Perform spot check audits on the ghoti looking for consistency, simplicity, adherence to standards and practices etc.
Sometimes it is simpler to implement small changes on the fly, rather than request them from the issue assignee, wait for them to be implemented, and then start the review again.

In these situations, particularly for Airways issues, if the changes you implemented affect the change overview section of the issue description, it is your responsibility to update the change overview accordingly.
Add comments for consumption by the original comitter:
1. For individual lines of ghoti:
  1. Scroll down the pull request details page in GitHub and locate a commit to be reviewed - click the link to see commit details including ghoti changes.
  2. Move the mouse pointer just to the right of the line number for a line of interesting ghoti - a white on blue "+" button should appear. Click it.
  3. Enter a comment relating to this line of ghoti.
2. As a preamble to the criticism:
  1. Scroll down the pull request details page in GitHub and locate where the user started a review text appears.
  2. Enter a preamble comment in the edit field immediately below.
Scroll down the pull request details page in GitHub and locate where the user started a review text appears.
Under the preamble comment text field are radio buttons to simply add a comment, approve the pull request, or request changes (seen in the above image). Select the appropriate radio button and click the Submit review button.
Update the pull request and remove the in progress label.
Advise the pull request submitter that feedback on the pull request has been provided. They will need to:
1. If changes were requested, address the suggestions and resubmit the pull request.
2. If the pull request was approved:
  1. Merge the pull request.
  2. Delete the branch.
  3. Remove the pull request label from the original issue.
  4. Close the issue (typically only if the feature branch was merged into a non-default branch and the issue was not automatically closed).

TODO

Nothing much.

Per aspera ad astra

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TG Consultancy Checklist

Tr Consultancy Checklist

Contents

Abstract

What to do when starting consultancy

What to do while performing consultancy

What to do before creating a pull request

Naming conventions

Properties

Titles

Fetchings

Unit tests

Webui

Streaming

Security

Database changes

Retrievers

Validators

Grouping Properties

Other

What to do when reviewing a pull request

TODO

Clone this wiki locally