Feedbacks

[azayati]

@vsellier feedbacks:

• It should not have uppercase on the directories https://github.com/exo-addons/app-center/tree/master/app-center-services/src/main/java/org/exoplatform/appCenter
• It should not have an app-center prefix on the module directory names
• Please add some logs on the exceptions on ApplicationDAO.java
• ApplicationsRestService.java Why not respecting the REST patterns on the rest service ?
• changelog-1.0.0.xml
  • The tables should be prefixed by something like AC_ or few letters allowing to identify them
  • https://github.com/exo-addons/app-center/blob/master/app-center-services/src/main/resources/db.changelogs/changelog-1.0.0.xml#L48 why 250 the usual size is 200 ? personally, I'm not convinced by USER_NAME what is the functional meaning of this column ?
• What is the expected number of applications ? The performance will probably be not very good with searches like 'like '%' + name + '%'
• Inconsistency between the namedquery getAppByNameOrTitle and it's parameters

@thomasdelhomenie feedbacks:

• There are a lot of npm dependencies ( https://github.com/exo-addons/app-center/blob/master/app-center-webapps/src/main/frontend/package.json#L34-L55 ), most of them are probably useless or should not be used:
  • vue-fontawesome : PLF font icons must be used (the font must be updated if new icons are required)
  • axios : most of the time fetch is sufficient and avoid to import a lib, for example in myApplications.vue fetch is sufficient IMO, no need for axios
  • bootstrap-vue : this lib has impact on the platform style (see https://github.com/exo-addons/gamification/pull/52#pullrequestreview-317495460), we should avoid to use it
  • jquery : the jquery AMD module must be used instead of importing a specific version
  • npm : why do we need npm here ?
  • terser : it seems to be not used
  • vue : the AMD module must be used instead
  • vue-d2b : the echart lib should be used now for charts (see https://community.exoplatform.com/portal/g/:spaces:platform_4/exo_platform/wiki/ADR-0003___Use_ECharts_as_the_platform_charts_library)
  • Others libs must be checked. Importing all these libs produces heavy js (adminSetup.bundle.js is 1.5Mb for example)
• The size of the URL field in the database is too short, URLs can be longer then 250 characters.
• The REST API endpoints must be described with Swagger annotations
• Using "like '%...%'" in named queries on permissions can cause issues IMO, for example if several groups have the same beginning (/group1 and /group10)
• In getAuthorizedApplications and getDefaultApplications, it can lead to a lot of requests to the database : if a user has 50 groups (for example 50 spaces), it will perform 50 queries to the database. See exampel https://github.com/exodev/social/blob/develop/component/core-jpa/src/main/java/org/exoplatform/social/core/jpa/storage/dao/jpa/query/RelationshipQueryBuilder.java
• IMO we should not retrieve the groups of the user in the DAO, but instead pass the list of the groups to the DAO and build a query with this list of groups.

[azayati]

personally, I'm not convinced by USER_NAME what is the functional meaning of this column ?

@vsellier The functional meaning of this column is that such application id is favorite for such user name. What is the problem with the user name ?

[azayati]

What is the expected number of applications ? The performance will probably be not very good with searches like 'like '%' + name + '%'

@vsellier No functional limit right now for created applications, but the favorite applications per user is limited by a global setting param defined by the admin, for which query the problem of performance can occur ?

[azayati]

This PR fixes:

@vsellier

• changelog-1.0.0.xml
• The tables should be prefixed by something like AC_ or few letters allowing to identify them
• https://github.com/exo-addons/app-center/blob/master/app-center- services/src/main/resources/db.changelogs/changelog-1.0.0.xml#L48 why 250 the usual size is 200
• Inconsistency between the namedquery getAppByNameOrTitle and it's parameters

@thomasdelhomenie

• The size of the URL field in the database is too short, URLs can be longer then 250 characters. (Set to 500)
• Using "like '%...%'" in named queries on permissions can cause issues IMO, for example if several groups have the same beginning (/group1 and /group10)
• IMO we should not retrieve the groups of the user in the DAO, but instead pass the list of the groups to the DAO and build a query with this list of groups.

[vsellier]

personally, I'm not convinced by USER_NAME what is the functional meaning of this column ?

@vsellier The functional meaning of this column is that such application id is favorite for such user name. What is the problem with the user name ? I thought this column is on the APPLICATION table, so I was asking why not naming it like OWNER or something like that. It this case your are right as the table name is enough to deduct the meaning

[azayati]

There are a lot of npm dependencies ( https://github.com/exo-addons/app-center/blob/master/app-center-webapps/src/main/frontend/package.json#L34-L55 ), most of them are probably useless or should not be used:

@thomasdelhomenie: All useless dependencies are removed, the war size is about 80Ko now