The developer and reviewer of a Pull Request must check the compliance with following guidelines documents:
Beyond those documents and guidelines, the reviewer must check the PR follows the best practices generally admitted in software development. If a practice is not described in the eXo Coding Best Practices document and is used in the PR, it is a good opportunity to discuss about it and add it if necessary in the document.
Unit Tests are essential to minimize regressions. The reviewer must check that unit tests are implemented to cover the related bug or the feature, especially:
-
Are the test titles adequately descriptive?
-
Are the key scenarios captured?
-
Are there enough edge cases covered for comfort?
-
Are the tests written with good assertions?
-
If a test fails, would it be easy to track down the error?
-
Are the tests independent from each others (especially if they manage data)?
The reviewer must also ensure that the unit tests coverage minimum ratio defined in Maven configuration is not decreased and advise to increase it if the test coverage has been improved.
The reviewer must check that the contribution does not introduce security issues, especially in the following areas:
-
Data/Input Validation of data from all untrusted sources
-
All input data must be validated before being used and/or stored in the system (UI forms, REST APIs inputs, …).
-
Authentication
-
Session Management
-
Authorization All the resources of the system (web pages, documents, REST APIs, …) must only be accessible by the authorized population.
-
Cryptography (Data at rest and in transit) Sensitive information must not be transmitted or persisted in clear text. Also, secure method must be used for cryptography (for example do not use MD5 to encode users’ passwords).
-
Error Handling / Information Leakage Sensitive information should not end up in error messages (logs, UI, …). For example do not include passwords or security tokens in logs.
-
Logging / Auditing Some operations require logging/auditing to allow to understand what happened during a security breach or detect security issues as they happen.
The reviewer must try to detect if the fix/feature could have significant bad impacts on the performance of the application. Performances issues can be expensive to find and fix, so it is important to raise any concern on this topic at this stage. Here are some examples of performance issues causes: too many database requests, slow database queries, missing index in the database, too many HTTP requests, …
In Front-End development, it’s important to consider the best solutions that reduce Memory & CPU consumption in Browser. In fact, a Web page executes only one single thread for all Vue apps, so it’s important to avoid adding a lot of stuff to do when mounting a Vue application (Read more about JS lifecycle). In fact, when adding heavy computing statements in the page loading phase, it will pause mounting all other Vue applications until the computing is finished, because of the single-threaded execution architecture of Web pages. Consequently, having one Vue app that isn’t sufficiently performant in a web page, especially for a TopBar application (that is present on all pages), will degrade the rendering performances of the whole page.
Knowing that the Vue applications rendering has to be made in an asynchronous way without impacting other applications present on the page. For example, when having information to retrieve from Server-End using REST API calls, a fetch call will be used. To ensure to have a good UX, a loading effect in the application itself has to be rendered the first time the Vue application is mounted, until all needed information is retrieved.
But, using the fetch API to retrieve information asynchronously from Server-End has a big drawback. In fact, a Browser allows limited concurrent HTTP requests for each domain (Domain Sharding). In a web page, that holds a lot of Vue applications, each one will retrieve information from Server-End using different REST endpoints. This can consequently lead to a very bad UX with a considerable idle time for HTTP requests. Fortunately, the Web Standards has evolved to take into consideration this complexity of new Web Applications especially by introducing the HTTP/2 protocol (Read more about benifits). To benefit from it, and to allow to increase the parallel concurrent requests to send to Server-End, we will have to preload and prefetch mandatory information that renders the Web Page (Read more).
The retrieved HTML DOM of eXo Platform contains references to the needed CSS files (computed from the list of Portlets being part of the displayed page). The fact that we have CSS links, in the retrieved DOM, will help the browser to determine which files to retrieve using HTTP/2 in an early stage of the Web Page rendering phase. But the Javascript files use a different mechanism based on RequireJS, which will retrieve JS files at the end of Web Page processing. This will lead to sending multiple HTTP requests that will not be optimized using HTTP/2. Thus, the JS and CSS files are preloaded to ensure retrieving those files once at the same time, even when having tens of them. Consequently, all JS files have to use AMD mechanism to be imported inside a page and any new module has to be imported as a dependency of a Portlet asset to ensure not losing time in the Web Page rendering phase. So, there are several bad practices to avoid such as, adding a link element inside a Vue application, or even worse when using requirejs in the render phase on a module that is not a dependency of a displayed Portlet.
The preload and prefetch mechanisms can be used for other types of HTTP requests. In fact, each Vue application needs assets to be fully displayed. In the general case, we will need:
-
A CSS file HTTP request for application stylesheet: handled by Portal API to be preloaded, no specific modifications to make while developing a Vue application
-
A JS file HTTP request to execute Vue application rendering: handled by Portal API to be preloaded, no specific modifications to make while developing a Vue application
-
An I18N JSON file HTTP request: handled by Portal API to be preloaded, no specific modifications to make while developing a Vue application. This type of asset is handled by a different caching mechanism (sessionStorage) to improve the number of concurrent requests sent to the server (see details)
-
A set of HTTP requests to retrieve user settings and/or initial data to display
A Vue application can need one or several HTTP requests to retrieve data from Server-End. It’s important to determine the priority of those requests and in which Web-Page rendering lifecycle, the request has to be sent. In fact, for the main application (The application that the user needs to retrieve its information, for example, the Stream application on /stream page), the requests have to have a higher priority comparing to permanent page applications (TopBar applications). In order to increase the priority of an HTTP request, a <link rel="preload" or <link rel="prefetch" can be used to increase the priority. Even more, you can add a HTTP Header that will allow retrieving the HTTP request at the same time as JS & CSS files (highest priority just after Document DOM downloading). BUT, the preloaded resources shouldn’t be slow queries, else the Web-Page rendering can be blocked for a while, and consequently, the Web-Page performances will degrade. As an example, the Stream page preloads the list of Activity Identifiers without content to let the Stream application load each activity apart. In order to improve activity loading, HTTP caching Headers have been used to efficiently cache activity content retrieving (Using eTag).
Sometimes, when not having a lot of interactions with the user on UI, (like for Wallet & Perkstore widgets), it’s more relevant to compute the HTML to display in Server-End using a simple JSP file without having to mount a Vue application for it and to retrieve its information using additional REST calls. Using Vue applications is relevant only when :
-
the computed information to display is slow: in this case, using a REST call to asynchronously retrieve information from Server-End will allow detaching heavy computing from the critical path of Web-Page DOM retrieving
-
there are complex UI components and user interactions in application DOM
In conclusion, to ensure having good Front-End performances when developing a Vue application, we have multiple techniques that can be applied, but there is no exact coding pattern that we can follow. Each application has its specificities and performances requirement has to be considered in the conception phase, else you can lose time to refactor code. In order to help developers continuously measure developed/maintained applications performances, tooling has been added to display application performances in the Browser console when the server is started in Dev Mode. To enable this, use:
-
Vue.createAppto create a new Vue application instead ofnew Vue -
use statement
this.$root.$applicationLoaded()when all Data is fetched and the UI has been displayed to the End-user.
The REST API calls count has increased over eXo Platform versions. This was a major evolution of eXo Platform product architecture introduced in version 6 which will reduce the classic Server-End stateful DOM rendering (that was made using JSP, Servlet, Portlet, JSF, Spring MVC…). As an advantage of this, the Rendering phase is decentralized and is deported on the Client-Side. Consequently, the Server will hold less information in its memory (Stateful UI Tree) and make less computing related to page rendering. As a significant drawback of this new architecture, making more parallel computing (parallel REST calls for each page) makes the multi-threading aspect more important to consider. In fact, a REST call has to be performant and must not hold any potential thread-blocking statement. In addition, the HTTP cache headers have to be considered for almost all REST calls. A REST call returns objects of type:
-
CSS/JS
-
Image
-
JSON representing a stored data
For CSS/JS and Images, it’s important to consider using a long-term cache strategy. By doing this, the Browser will always retrieve the resource from the local cache (Disk or Memory cache) and will not fetch the resource from Server-End again. If the resource can be dynamic, such as illustration on news or Application Center thumbnail, you can add a suffix to the URL that references the lastModifiedDate property of the image (See example)
For the JSON objects retrieved using REST API, you can consider using an ETag to cache objects in browser and to verify that it hasn’t been modified since last retrieval. This strategy is very relevant when retrieving information that is not frequently changed, such as an activity that once written, it will not change most of the time. By using this strategy, you will avoid to download the Object content from Server-end which will save Bandwidth consumption (consequently save time) and even save computing time in Server-End for DTO to JSON transformation (See example).
The maintainability measure how easy it is to make changes in a code base (fixing bugs, adding new features, replacing a faulty or obsolete component, …). This means:
-
Tests are implemented to ensure a good test coverage and help understand how code should behave
-
Classes and methods have a clear and single responsibility
-
Classes, methods and variables names are self-descriptive and/or well documented
-
Classes and methods are short
-
Cyclomatic complexity of a method should be low
-
Components are loosely coupled
-
Code duplication should be avoided
-
Code must respect formatting rules
More generally, if it was hard for the reviewer to understand, the code should probably be reworked to make it easy to understand since it means it will be hard to understand for next developers in the future.
When problems occur in production, it is generally not possible to debug or to update easily the code to find the cause. Therefore, the developer must try to anticipate the potential issues and provide the information and/or tools to help finding the error cause and fixing it. The reviewer must evaluate if the contribution contains the right elements to help this troubleshooting. Here are some examples of question to answer:
-
Is there enough logs ?
-
Do the logs have the appropriate level ?
-
If relevant, in case of problem, is there any tool (JMX bean, …) to gather more information or to recover ?
Any contribution must be considered to be installed on an existing environment. In such a case it must be ensured that the upgrade is done as transparently as possible:
If data are impacted, an upgrade plugin must be developed
If any configuration change is required, the documentation and the upgrade notes must be updated