This repository was archived by the owner on Jan 2, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1
This repository was archived by the owner on Jan 2, 2024. It is now read-only.
Feedbacks #2
Copy link
Copy link
Open
Description
@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
getAppByNameOrTitleand 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 Us51 i18 n manage rules gamification#52 (review)), 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.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels