add isImported And categories to project tab #1822
Conversation
CarlosQ96
force-pushed
the
hotfix_project_additional_attributes
branch
from
4767081
to
8d30b1e
Compare
WalkthroughThe changes introduce an eager loading strategy for project categories in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
src/entities/project.ts (1)
220-220: Eager loading enabled forcategories. Monitor performance impact.Enabling eager loading for the
categoriesproperty ensures that associatedCategoryentities are automatically loaded when aProjectentity is retrieved. This can simplify the application logic by eliminating the need for explicit loading ofCategoryentities.However, please keep an eye on the performance impact, especially if
Projectentities are frequently loaded and the associatedCategorydata is not always needed. Consider using alternative loading strategies, such as lazy loading or explicit loading, if the eager loading causes performance issues.src/server/adminJs/tabs/projectsTab.ts (2)
450-455: LGTM!The
extractCategoryIdsfunction correctly extracts category IDs from the request payload by filtering and mapping the relevant keys.Optionally, consider fixing the minor formatting issues flagged by the static analysis tool:
-export const extractCategoryIds = (payload: any) => { - if (!payload) return; - return Object.keys(payload) - .filter(key => key.startsWith('categoryIds.')) - .map(key => payload[key]); +export const extractCategoryIds = (payload: any) => { + if (!payload) return; + return Object.keys(payload) + .filter(key => key.startsWith('categoryIds.')) + .map(key => payload[key]); +};Tools
GitHub Check: test
[failure] 450-450:
Delete·
[failure] 451-451:
Delete··
[failure] 452-452:
Delete··
[failure] 453-453:
Delete··
[failure] 454-454:
Delete··
[failure] 455-455:
Insert;
497-505: Looks good!The query correctly fetches the categories associated with the project, ordered by name. The fetched categories are then assigned to the
categoriesvariable.Optionally, consider fixing the minor formatting issues flagged by the static analysis tool:
let categories; if (project) { - categories = await Category - .createQueryBuilder('category') - .innerJoin('category.projects', 'projects') - .where('projects.id = :id', { id: project.id }) - .orderBy('category.name', 'ASC') - .getMany(); + categories = await Category.createQueryBuilder('category') + .innerJoin('category.projects', 'projects') + .where('projects.id = :id', { id: project.id }) + .orderBy('category.name', 'ASC') + .getMany(); }Tools
GitHub Check: test
[failure] 499-499:
Delete⏎············
[failure] 501-501:
Delete······
[failure] 502-502:
Delete······
[failure] 503-503:
Delete······
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/entities/project.ts (1 hunks)
- src/server/adminJs/tabs/components/ProjectCategories.tsx (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (11 hunks)
Additional context used
GitHub Check: test
src/server/adminJs/tabs/projectsTab.ts
[failure] 450-450:
Delete·
[failure] 451-451:
Delete··
[failure] 452-452:
Delete··
[failure] 453-453:
Delete··
[failure] 454-454:
Delete··
[failure] 455-455:
Insert;
[failure] 499-499:
Delete⏎············
[failure] 501-501:
Delete······
[failure] 502-502:
Delete······
[failure] 503-503:
Delete······
Additional comments not posted (10)
src/server/adminJs/tabs/components/ProjectCategories.tsx (4)
1-4: LGTM!The imports are appropriate and there are no unused imports.
5-26: LGTM!The component implementation looks good:
- It safely retrieves project categories using optional chaining.
- It handles missing category names by defaulting to an empty string.
- The rendering logic is clear and concise.
28-28: LGTM!The component is correctly exported with the
withThemehigher-order component.
1-28: Overall assessment: The component looks great!The
ProjectCategoriescomponent is a valuable addition to the project tab functionality. It aligns with the PR objectives by providing a clear and organized view of project categories, making it easier for users to identify and differentiate between them.The component is well-structured, utilizing the theme and design system components effectively. It handles edge cases gracefully and contributes to the overall goal of enhancing project management capabilities.
Great job on implementing this component!
src/server/adminJs/tabs/projectsTab.ts (6)
519-523: LGTM!The code correctly adds the fetched
categoriesto the response record parameters under bothcategoryIdsandcategoriesproperties.
856-880: Looks good!The
categoryIdsfield is correctly defined as a reference array to theCategoryentity. The visibility settings and custom component usage are appropriately configured.The
availableValuesmethod fetches active categories ordered by name, providing a dynamic selection list.
974-989: LGTM!The
beforeandafterhooks correctly handle the category data during project creation.The
beforehook processes thecategoriespayload by converting the category IDs to objects compatible with the ORM.The
afterhook retrieves the created project, extracts the selected category IDs, and associates them with the project using thesaveCategoriesfunction.
1018-1023: Looks good!The code correctly deletes existing category associations for the project before updating.
It executes a raw SQL DELETE query on the
project_categories_categoryjoin table, removing rows where theprojectIdmatches the current project's ID.This ensures a clean slate for updating the project with the new category selections.
1225-1231: LGTM!The code correctly saves the category associations after updating the project.
It extracts the selected category IDs from the request record params using the
extractCategoryIdsfunction.It then calls the
saveCategoriesfunction, passing theprojectobject and extractedcategoryIds, to associate the categories with the project.The
saveCategoriesfunction is executed concurrently with other refresh operations usingPromise.all, ensuring efficient saving of category associations along with other post-update tasks.
1428-1444: Looks good!The
saveCategorieshelper function correctly handles saving category associations for a project.It first cleans up any existing category associations by executing a raw SQL DELETE query on the join table.
It then retrieves the selected categories based on the provided
categoryIdsusing TypeORM's query builder.The retrieved categories are assigned to the
project.categoriesproperty, and the project is saved to persist the associations.The function includes appropriate guard clauses to handle edge cases and ensure data integrity.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectsTab.ts (1)
450-455: Refactor theextractCategoryIdsfunction to use a more concise and efficient approach.The current implementation can be simplified using the
Object.entries()method and array destructuring. Here's a refactored version:export const extractCategoryIds = (payload: any) => { if (!payload) return; return Object.entries(payload) .filter(([key]) => key.startsWith('categoryIds.')) .map(([, value]) => value); };This refactored code achieves the same functionality but with a more concise and readable approach.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/server/adminJs/tabs/components/CustomQfRoundMultiUpdateComponent.tsx (1 hunks)
- src/server/adminJs/tabs/components/ProjectCategories.tsx (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (11 hunks)
Files skipped from review due to trivial changes (1)
- src/server/adminJs/tabs/components/CustomQfRoundMultiUpdateComponent.tsx
Files skipped from review as they are similar to previous changes (1)
- src/server/adminJs/tabs/components/ProjectCategories.tsx
Additional comments not posted (5)
src/server/adminJs/tabs/projectsTab.ts (5)
497-504: LGTM!The code correctly retrieves the associated categories for a project and orders them by name in ascending order. The
Categoryentity is properly used with thecreateQueryBuildermethod to construct the query.
518-522: Verify consistency betweencategoryIdsandcategoriesparameters.The code assigns the retrieved
categoriesto bothresponse.record.params.categoryIdsandresponse.record.params.categories. While this may be intentional, it's worth double-checking ifcategoryIdsshould contain just the IDs of the categories instead of the entire category objects. Ensure that the naming and usage of these parameters are consistent throughout the codebase to avoid confusion.
855-878: LGTM!The
categoryIdsfield is properly defined as a reference type with the following characteristics:
- It is an array field, allowing multiple category selections.
- It references the
Categoryentity.- It is configured to be visible in the show and edit views.
- It uses a custom component for displaying project categories.
- The
availableValuesmethod is implemented to dynamically fetch active categories from the database and provide a selectable list.The code follows best practices and provides a clear configuration for the
categoryIdsfield.
972-989: Ensure data consistency and handle potential errors.The code in the
beforeandafterhooks of thenewaction handles the processing of category data for new projects. However, there are a few points to consider:
In the
beforehook, the code assumes thatrequest.payload.categoriesis always an array of strings. It would be safer to add a type check or handle the case where it might beundefinedto prevent potential runtime errors.In the
afterhook, the code retrieves the newly created project usingProject.findOne()with theidfromrequest.record.id. However, it doesn't handle the case where the project might not be found. Consider adding a null check or error handling to ensure data consistency.The
saveCategoriesfunction is called with the retrieved project and the extracted category IDs. However, there's no error handling or logging in case thesaveCategoriesfunction throws an error. Consider wrapping the function call in atry-catchblock to handle any potential errors gracefully.By addressing these points, you can improve the robustness and reliability of the code when processing category data for new projects.
1431-1449: LGTM!The
saveCategoriesfunction is implemented correctly and follows best practices:
It checks if the
projectandcategoryIdsparameters are valid before proceeding, ensuring that the function doesn't execute unnecessarily.It deletes existing category associations for the project using a SQL query with the
project.idas a parameter. This ensures that old associations are removed before saving new ones.It retrieves the
Categoryentities based on the providedcategoryIdsusing thecreateQueryBuildermethod and theINoperator, ensuring that only the specified categories are fetched.It assigns the retrieved
categoriesto theproject.categoriesproperty and saves the project usingproject.save(), persisting the changes to the database.The function is well-structured, uses appropriate error handling, and follows a clear flow of operations. It effectively handles the saving of categories for a project.
| if (project) { | ||
| await Category.query( | ||
| ` | ||
| DELETE FROM project_categories_category | ||
| WHERE "projectId" = $1 | ||
| `, | ||
| [project.id], | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Verify the necessity of deleting existing category associations.
The code deletes existing category associations for a project before updating the categories. While this ensures that old associations are removed, it's worth considering the following:
-
If the update operation fails or is interrupted after deleting the associations but before saving the new associations, the project might end up with no associated categories. This could lead to inconsistencies in the data.
-
Deleting and recreating associations for every update operation might not be the most efficient approach, especially if the categories haven't changed.
Consider an alternative approach where you compare the existing associated categories with the updated categories and only perform the necessary deletions and additions. This way, you can minimize the risk of data inconsistencies and improve the efficiency of the update operation.
If deleting all existing associations is indeed required, ensure that the update operation is performed atomically or within a transaction to maintain data integrity.
There was a problem hiding this comment.
Its risky but required. Admin js does a under the hood update. I need to clean up
There was a problem hiding this comment.
@CarlosQ96 Understood. Thank you for the clarification.
(_/)
(•_•) Hmm...
Learnings added
Learnt from: CarlosQ96
PR: Giveth/impact-graph#1822
File: src/server/adminJs/tabs/projectsTab.ts:1018-1027
Timestamp: 2024-09-19T01:02:30.617Z
Learning: When updating project categories in AdminJS, it's necessary to delete existing category associations before saving new ones due to how AdminJS handles updates under the hood.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const categoryIds = extractCategoryIds(request.record.params); | ||
|
|
||
| await Promise.all([ | ||
| refreshUserProjectPowerView(), | ||
| refreshProjectFuturePowerView(), | ||
| refreshProjectPowerView(), | ||
| saveCategories(project!, categoryIds || []), |
There was a problem hiding this comment.
Consider moving the saveCategories function call outside the Promise.all.
The code currently includes the saveCategories function call within the Promise.all block along with other asynchronous operations. While this ensures that all operations are executed concurrently, it's worth considering the following:
-
The
saveCategoriesfunction operates on the project's categories, which is a separate concern from refreshing user project power views and project power views. -
If any of the operations within the
Promise.allblock fails, it will cause the entire block to reject, potentially leading to inconsistencies in the data if thesaveCategoriesfunction is not executed.
Consider moving the saveCategories function call outside the Promise.all block and handling it separately. This way, you can ensure that the project's categories are saved independently of the other operations, improving the reliability and maintainability of the code.
| const categoryIds = extractCategoryIds(request.record.params); | |
| await Promise.all([ | |
| refreshUserProjectPowerView(), | |
| refreshProjectFuturePowerView(), | |
| refreshProjectPowerView(), | |
| saveCategories(project!, categoryIds || []), | |
| await saveCategories(project!, categoryIds || []); | |
| await Promise.all([ | |
| refreshUserProjectPowerView(), | |
| refreshProjectFuturePowerView(), | |
| refreshProjectPowerView(), | |
| ]); |
By separating the concerns and handling them independently, you can improve the robustness and clarity of the code.
CarlosQ96
force-pushed
the
hotfix_project_additional_attributes
branch
from
b526123
to
4b8d308
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectsTab.ts (1)
980-989: Ensure the project is saved after updating categories.The
afterhook for thenewaction retrieves the newly created project and extracts the category IDs from the request payload. It then calls thesaveCategoriesfunction to associate the categories with the project.Consider adding a null check for the
projectvariable before callingsaveCategoriesto handle cases where the project is not found.const project = await Project.findOne({ where: { id: request?.record?.id }, }); if (project) { const categoryIds = extractCategoryIds(request.record.params); await saveCategories(project, categoryIds || []); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/server/adminJs/tabs/components/CustomQfRoundMultiUpdateComponent.tsx (1 hunks)
- src/server/adminJs/tabs/components/ProjectCategories.tsx (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (11 hunks)
Files skipped from review due to trivial changes (1)
- src/server/adminJs/tabs/components/CustomQfRoundMultiUpdateComponent.tsx
Files skipped from review as they are similar to previous changes (1)
- src/server/adminJs/tabs/components/ProjectCategories.tsx
Additional comments not posted (3)
src/server/adminJs/tabs/projectsTab.ts (3)
450-455: LGTM!The
extractCategoryIdsfunction correctly extracts the category IDs from the request payload. It filters the payload keys that start with'categoryIds.'and maps them to their corresponding values.
855-878: LGTM!The
categoryIdsfield is correctly defined as a reference type with the following properties:
- It is an array field referencing the
Categoryresource.- It is visible in the show and edit views.
- It uses a custom component
ProjectCategoriesfor displaying the categories.- The
availableValuesmethod fetches the active categories from the database and returns them as selectable options.
972-979: Verify the payload structure and parsing logic.The
beforehook for thenewaction checks if thecategoriesproperty exists in the request payload. If it does, it maps the category IDs to an array of objects with theidproperty.To ensure the correctness of this logic, please verify the following:
- The structure of the
request.payloadobject and the expected format of thecategoriesproperty.- The parsing logic assumes that
request.payload.categoriesis an array of strings representing category IDs. Confirm that this assumption aligns with the actual payload structure.Verification successful
Payload structure and parsing logic verified.
The
beforehook for thenewaction insrc/server/adminJs/tabs/projectsTab.tscorrectly processes thecategoriesproperty of the request payload. This operation appears to be unique to this admin JS tab and serves a specific purpose:
- It checks if
request.payload.categoriesexists.- If present, it transforms the array of string IDs into an array of objects with numeric IDs.
This transformation is likely necessary to match the database schema or ORM requirements for category associations. The parsing logic correctly assumes that
request.payload.categoriesis an array of strings representing category IDs.For improved type safety, consider adding a type guard:
if (Array.isArray(request.payload.categories)) { request.payload.categories = request.payload.categories.map(id => ({ id: parseInt(id, 10) })); }This change would ensure that the operation is only performed on array inputs, preventing potential runtime errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of the request payload and the `categories` property. # Test 1: Search for the usage of the `categories` property in the codebase. # Expect: Consistent usage and structure of the `categories` property. rg --type typescript $'request\.payload\.categories' -A 5 # Test 2: Search for the creation of new projects in the codebase. # Expect: Consistent payload structure when creating new projects. rg --type typescript $'Project\.create\(' -A 10Length of output: 177
Script:
#!/bin/bash # Search for usage of payload.categories in .ts and .js files echo "Searching for payload.categories usage:" fd '\.(ts|js)$' | xargs rg 'payload\.categories' -n -A 10 echo "\n---\n" # Search for Project.create in .ts and .js files echo "Searching for Project.create usage:" fd '\.(ts|js)$' | xargs rg 'Project\.create' -n -A 10Length of output: 36500
| async function saveCategories(project: Project, categoryIds?: string[]) { | ||
| if (!project) return; | ||
| if (!categoryIds || categoryIds?.length === 0) return; | ||
|
|
||
| await Category.query( | ||
| ` | ||
| DELETE FROM project_categories_category | ||
| WHERE "projectId" = $1 | ||
| `, | ||
| [project.id], | ||
| ); | ||
|
|
||
| const categories = await Category.createQueryBuilder('category') | ||
| .where('category.id IN (:...ids)', { ids: categoryIds }) | ||
| .getMany(); | ||
|
|
||
| project.categories = categories; | ||
| await project.save(); | ||
| } |
There was a problem hiding this comment.
Consider handling the case when project categories are unchanged.
The saveCategories function deletes all existing category associations for a project and then saves the new categories. While this approach ensures that the project categories are updated correctly, it may be inefficient if the categories haven't changed.
Consider comparing the existing associated categories with the updated categories and only perform the necessary deletions and additions. This optimization can reduce the number of database queries and improve performance.
| async function saveCategories(project: Project, categoryIds?: string[]) { | |
| if (!project) return; | |
| if (!categoryIds || categoryIds?.length === 0) return; | |
| await Category.query( | |
| ` | |
| DELETE FROM project_categories_category | |
| WHERE "projectId" = $1 | |
| `, | |
| [project.id], | |
| ); | |
| const categories = await Category.createQueryBuilder('category') | |
| .where('category.id IN (:...ids)', { ids: categoryIds }) | |
| .getMany(); | |
| project.categories = categories; | |
| await project.save(); | |
| } | |
| async function saveCategories(project: Project, categoryIds?: string[]) { | |
| if (!project) return; | |
| if (!categoryIds || categoryIds?.length === 0) return; | |
| const existingCategoryIds = project.categories.map(category => category.id.toString()); | |
| const addedCategoryIds = categoryIds.filter(id => !existingCategoryIds.includes(id)); | |
| const removedCategoryIds = existingCategoryIds.filter(id => !categoryIds.includes(id)); | |
| if (removedCategoryIds.length > 0) { | |
| await Category.query( | |
| ` | |
| DELETE FROM project_categories_category | |
| WHERE "projectId" = $1 AND "categoryId" IN (${removedCategoryIds.map((_, i) => `$${i + 2}`).join(', ')}) | |
| `, | |
| [project.id, ...removedCategoryIds], | |
| ); | |
| } | |
| if (addedCategoryIds.length > 0) { | |
| const addedCategories = await Category.createQueryBuilder('category') | |
| .where('category.id IN (:...ids)', { ids: addedCategoryIds }) | |
| .getMany(); | |
| project.categories = [...project.categories, ...addedCategories]; | |
| await project.save(); | |
| } | |
| } |
By comparing the existing and updated category IDs, you can selectively delete the removed categories and add the new categories. This approach minimizes the number of database operations and improves efficiency.
Related to: Giveth/giveth-dapps-v2#4718
Allow admins to add categories to project and mark one as isImported
Summary by CodeRabbit
New Features
eagerloading strategy for project categories to enhance performance.Bug Fixes
Documentation