feat(server): Projects Recycle bin #1169
Conversation
WalkthroughThe pull request introduces updates to enhance project management functionality within the server codebase. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
Poem
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
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
hexaforce
requested review from
airslice,
mkumbobeaty and
pyshx
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (26)
server/Makefile (1)
23-25: LGTM! Consider enhancing the comment for clarity.The new
failchecktarget is a valuable addition to the Makefile. It provides a way to run tests with fail-fast and single-threaded execution, which is useful for quickly identifying and isolating test failures.Consider updating the comment to be more descriptive:
-# make failcheck TEST_DIR=./internal/usecase/interactor +# Example usage: make failcheck TEST_DIR=./internal/usecase/interactor +# This runs tests in the specified directory with fail-fast and no parallel execution.This change would provide more context about the target's purpose and usage.
server/internal/usecase/repo/project.go (3)
26-26: LGTM! Consider adding documentation.The new
FindDeletedByWorkspacemethod is a good addition that aligns with the PR objectives of introducing adeletedflag to the project management system. The method signature is consistent with other methods in the interface.Consider adding a comment to document the purpose and behavior of this method, especially regarding the
deletedflag. For example:// FindDeletedByWorkspace retrieves all projects marked as deleted for the given workspace. // It returns a slice of deleted projects and an error if the operation fails. FindDeletedByWorkspace(context.Context, accountdomain.WorkspaceID) ([]*project.Project, error)
Line range hint
1-71: Summary and Next StepsThe changes in this file successfully introduce the
FindDeletedByWorkspacemethod to theProjectinterface, aligning with the PR objectives of implementing a projects recycle bin feature. The existing code, including theIterateProjectsByWorkspacefunction, remains compatible with this new addition.Next steps:
- Implement the
FindDeletedByWorkspacemethod in the concrete types that implement theProjectinterface.- Update the GraphQL schema and resolvers to utilize the new method for the
DeletedProjectsquery mentioned in the PR objectives.- Consider adding a new
IterateDeletedProjectsByWorkspacefunction if needed.- Ensure that all references to
includeArchivedhave been properly removed or updated throughout the codebase.- Add appropriate unit tests for the new functionality.
Verification of
includeArchivedRemoval Revealed Remaining ImplementationsThe
includeArchivedflag is still present in several Go files, indicating incomplete removal:
server/internal/adapter/gql/resolver_team.goserver/internal/adapter/gql/generated.go🔗 Analysis chain
Line range hint
33-71: Consider additional improvements and verifications.While the
IterateProjectsByWorkspacefunction remains unchanged, the introduction of thedeletedflag and the removal of theincludeArchivedflag (as mentioned in the PR objectives) might require some adjustments:
Consider adding a new function
IterateDeletedProjectsByWorkspaceto handle iteration over deleted projects, similar toIterateProjectsByWorkspace.Verify that the removal of the
includeArchivedflag from the GetProjects GraphQL query doesn't break any existing functionality.To verify the impact of removing the
includeArchivedflag, you can run the following script:This script will help ensure that all references to
includeArchivedhave been properly removed or updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of 'includeArchived' in GraphQL queries # Test: Search for 'includeArchived' in GraphQL files rg --type graphql 'includeArchived' # Test: Search for 'includeArchived' in Go files (in case it's used in resolvers) rg --type go 'includeArchived'Length of output: 1684
server/internal/usecase/interfaces/project.go (3)
49-49: LGTM! Consider adding a comment for clarity.The addition of the
Deleted *boolfield to theUpdateProjectParamstruct aligns well with the PR objective of introducing a deletion flag for projects. The use of a pointer to a boolean is consistent with other similar fields in the struct.Consider adding a brief comment to explain the purpose of this field, for example:
Deleted *bool // Indicates whether the project is marked as deleted (true) or not (false)
67-67: LGTM! Consider adding pagination for consistency.The addition of the
FindDeletedByWorkspacemethod to theProjectinterface aligns well with the PR objective of introducing a new query to retrieve deleted projects. The method signature is consistent with other similar methods in the interface.For consistency with the
FindByWorkspacemethod, consider adding pagination support:FindDeletedByWorkspace(context.Context, accountdomain.WorkspaceID, *usecasex.Pagination, *usecase.Operator) ([]*project.Project, *usecasex.PageInfo, error)This would allow for more efficient querying of large numbers of deleted projects.
Remove or Deprecate the
ArchivedFieldThe
Archivedfield is still actively used in several parts of the codebase, including:
server/internal/infrastructure/mongo/mongodoc/project.goserver/internal/adapter/gql/resolver_mutation_project.goserver/internal/adapter/gql/gqlmodel/convert_project.goserver/internal/adapter/gql/generated.goGiven the removal of the
includeArchivedflag from the GetProjects GraphQL query, it's recommended to deprecate or remove theArchivedfield to maintain consistency and avoid potential conflicts or confusion.🔗 Analysis chain
Line range hint
1-85: Clarify the relationship between "deleted" and "archived" statesWith the introduction of the "deleted" state, it's important to clarify its relationship with the existing "archived" state. Additionally, the PR objectives mention removing the
includeArchivedflag from the GetProjects GraphQL query, but theArchivedfield is still present in theUpdateProjectParamstruct.Please clarify the following points:
- How do the "deleted" and "archived" states interact? Are they mutually exclusive, or can a project be both archived and deleted?
- Is the
Archivedfield still in use, given the removal ofincludeArchivedfrom the GraphQL query? If not, should it be deprecated or removed?- Are there any plans to update other methods in the
Projectinterface to handle the new "deleted" state?To help verify the usage of the
Archivedfield, you can run the following script:This will help determine if the
Archivedfield is still being used in non-test code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the Archived field in the codebase rg --type go 'Archived\s*:' -g '!*_test.go'Length of output: 636
server/internal/infrastructure/mongo/mongodoc/project.go (2)
Line range hint
84-134: AddDeletedfield toModelmethodThe
Modelmethod is not currently including theDeletedstatus when constructing theproject.Projectinstance. This omission could lead to data loss or inconsistencies when converting betweenProjectDocumentandProject.Please update the
Modelmethod to include theDeletedstatus. Add the following line to theproject.New()chain, just before theBuild()call:Starred(d.Starred). + IsDeleted(d.Deleted). Build()This change ensures that the deletion status is preserved when converting from
ProjectDocumenttoProject.
includeArchivedflag still present in multiple filesThe
includeArchivedflag was intended to be removed from the GetProjects GraphQL query. However, references toincludeArchivedwere found in the following files:
web/src/services/gql/__gen__/graphql.tsserver/gql/workspace.graphqlserver/internal/adapter/gql/resolver_team.goserver/internal/adapter/gql/generated.goPlease ensure that all instances of
includeArchivedare removed or updated accordingly to maintain consistency with the PR objectives.🔗 Analysis chain
Line range hint
1-134: Verify removal ofincludeArchivedflagThe PR objectives mention removing the
includeArchivedflag from the GetProjects GraphQL query. However, this change is not reflected in the current file. To ensure consistency across the codebase:Please run the following script to check for any remaining references to
includeArchived:If any results are found, please ensure that these references are updated or removed as necessary to maintain consistency with the removal of the
includeArchivedflag.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to 'includeArchived' in the codebase # Test: Search for 'includeArchived' in all files rg --type-add 'code:*.{go,graphql,js,ts,jsx,tsx}' --type code 'includeArchived'Length of output: 1906
server/internal/adapter/gql/loader_project.go (1)
104-124: LGTM! Consider a minor optimization for clarity.The implementation of
FindDeletedByWorkspacelooks good and aligns well with the PR objectives. It follows a consistent pattern with the existingFindStarredByWorkspacemethod, which is great for maintainability.For improved clarity and consistency with other methods in this file (e.g.,
FindByWorkspace), consider using named return values and separating the edge creation. Here's a suggested refactor:-func (c *ProjectLoader) FindDeletedByWorkspace(ctx context.Context, wsID gqlmodel.ID) (*gqlmodel.ProjectConnection, error) { +func (c *ProjectLoader) FindDeletedByWorkspace(ctx context.Context, wsID gqlmodel.ID) (connection *gqlmodel.ProjectConnection, err error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } res, err := c.usecase.FindDeletedByWorkspace(ctx, tid, getOperator(ctx)) if err != nil { return nil, err } nodes := make([]*gqlmodel.Project, 0, len(res)) + edges := make([]*gqlmodel.ProjectEdge, 0, len(res)) for _, p := range res { - nodes = append(nodes, gqlmodel.ToProject(p)) + prj := gqlmodel.ToProject(p) + nodes = append(nodes, prj) + edges = append(edges, &gqlmodel.ProjectEdge{ + Node: prj, + Cursor: usecasex.Cursor(prj.ID), + }) } - return &gqlmodel.ProjectConnection{ + connection = &gqlmodel.ProjectConnection{ + Edges: edges, Nodes: nodes, TotalCount: len(nodes), - }, nil + } + return }This refactor improves consistency with other methods and provides more information in the returned
ProjectConnection, which might be useful for pagination in the future.server/pkg/project/project.go (1)
187-189: LGTM with a minor suggestion: Addition ofSetDeleted()methodThe
SetDeleted()method has been correctly implemented as a setter for theisDeletedfield. It follows Go conventions and maintains proper encapsulation.For consistency with other setter methods in this file (e.g.,
SetArchived()), consider renaming this method toSetIsDeleted().-func (p *Project) SetDeleted(isDeleted bool) { +func (p *Project) SetIsDeleted(isDeleted bool) { p.isDeleted = isDeleted }server/internal/infrastructure/memory/project.go (1)
108-128: LGTM with minor suggestions.The
FindDeletedByWorkspacemethod is well-implemented and aligns with the PR objectives. It follows the existing code patterns and correctly handles thread safety and permission checks. Here are a few suggestions for improvement:
- Consider implementing pagination for large datasets.
- Add explicit error handling beyond the permission check.
Consider adding a comment to explain the method's purpose and behavior:
// FindDeletedByWorkspace retrieves all deleted projects for a given workspace. // It returns the projects sorted by their update time in descending order. // This method is thread-safe and respects workspace read permissions. func (r *Project) FindDeletedByWorkspace(ctx context.Context, id accountdomain.WorkspaceID) ([]*project.Project, error) { // ... (existing implementation) }server/internal/infrastructure/mongo/project.go (1)
112-123: LGTM! Consider adding a comment for consistency.The implementation of
FindDeletedByWorkspacelooks good and aligns well with the PR objectives. It follows the same pattern asFindStarredByWorkspaceand correctly handles access control and query construction.For consistency with other methods in the file, consider adding a brief comment describing the method's purpose:
// FindDeletedByWorkspace retrieves all deleted projects for the given workspace func (r *Project) FindDeletedByWorkspace(ctx context.Context, id accountdomain.WorkspaceID) ([]*project.Project, error) { // ... (existing implementation) }server/internal/usecase/interactor/project_test.go (3)
Line range hint
67-68: LGTM! Consider adding a test case for defaultCoreSupportvalue.The changes to
TestProject_Createfunction look good. The newCoreSupportfield is correctly added to theCreateProjectParamstructure and the expected project object. Both normal (CoreSupport: false) and experimental (CoreSupport: true) scenarios are covered.Consider adding a test case where the
CoreSupportfield is not explicitly set in theCreateProjectParam. This would ensure that the default behavior is also tested.Also applies to: 102-103, 114-115
221-221: LGTM! Consider adding test cases for deleted projects.The addition of the
isDeletedfield to the expected project data structure inTestImportProjectfunction is correct and aligns with the PR objectives. The default value offalsefor a newly imported project is appropriate.To ensure comprehensive testing, consider adding the following test cases:
- Importing a project with
isDeletedset totruein the input data.- Verifying that the
DeletedProjectsquery returns only projects withisDeletedset totrue.- Testing the behavior of the
UpdateProjectmutation when modifying theisDeletedflag.These additional test cases would help ensure the robustness of the new deletion flag feature.
Line range hint
1-238: Overall changes align well with PR objectives.The modifications to
TestProject_CreateandTestImportProjectfunctions effectively incorporate the newCoreSupportandisDeletedfields, respectively. These changes are consistent with the PR objectives of introducing a deletion flag and enhancing the project management system.To further strengthen the implementation:
- Ensure that the
DeletedProjectsquery is thoroughly tested in a separate test function.- Verify that the removal of the
includeArchivedflag from theGetProjectsquery is reflected in the relevant test cases.- Consider adding integration tests that cover the interaction between the new deletion flag and other project operations (e.g., archiving, updating, etc.).
These steps will help maintain the robustness of the project management system as new features are introduced.
server/internal/adapter/gql/resolver_mutation_project.go (1)
Line range hint
117-129: Consider clarifying the permanent nature ofDeleteProject.Given the introduction of the soft-delete functionality with the new
deletedflag, it might be beneficial to emphasize that theDeleteProjectmethod performs a permanent deletion. Consider either:
- Renaming the method to
PermanentlyDeleteProjectfor clarity, or- Adding a comment above the method to highlight its permanent nature.
This change would help prevent accidental use of this method when a soft-delete is intended.
Example implementation:
+// PermanentlyDeleteProject removes the project from the database without possibility of recovery. +// For soft-delete functionality, use UpdateProject with the deleted flag instead. func (r *mutationResolver) DeleteProject(ctx context.Context, input gqlmodel.DeleteProjectInput) (*gqlmodel.DeleteProjectPayload, error) { // ... (existing implementation) }server/e2e/gql_project_test.go (2)
503-533: LGTM: Deleted projects query and verificationThe implementation of the deleted projects query and the subsequent verification looks correct. It properly checks the total count of deleted projects and verifies the name of the deleted project.
A minor suggestion for improved readability:
Consider extracting the GraphQL query string into a separate constant at the top of the file. This would make the test more maintainable and easier to read. For example:
const deletedProjectsQuery = ` query GetDeletedProjects($teamId: ID!) { deletedProjects(teamId: $teamId) { nodes { id name isDeleted } totalCount } }` // Then in the test function: requestBody = GraphQLRequest{ OperationName: "GetDeletedProjects", Query: deletedProjectsQuery, Variables: map[string]any{ "teamId": wID, }, }
458-534: Suggestions for additional test coverageThe
TestDeleteProjectsfunction provides good coverage for the new deletion flag functionality. However, to fully align with the PR objectives, consider adding the following test cases:
- Verify that the
GetProjectsquery no longer includes theincludeArchivedflag.- Test that the
deleteProjectmutation still permanently removes a project from the database.These additional tests will ensure that all aspects of the PR objectives are covered and that existing functionality remains intact.
Would you like assistance in drafting these additional test cases?
server/internal/usecase/interactor/project.go (3)
92-94: LGTM! Consider adding input validation.The
FindDeletedByWorkspacefunction is a good addition that aligns with the PR objectives. It correctly fetches deleted projects for a given workspace.Consider adding input validation for the
idparameter to ensure it's not empty before callingprojectRepo.FindDeletedByWorkspace.
235-237: LGTM! Consider handling side effects of deletion.The addition of the
Deletedflag handling in theUpdatefunction is correct and aligns with the PR objectives.Consider if there are any side effects of marking a project as deleted that should be handled here, such as:
- Notifying relevant users or systems
- Updating related entities (e.g., scenes, assets)
- Triggering any cleanup processes
If such side effects exist, it might be worth implementing them or adding a TODO comment for future implementation.
Removal of
includeArchivedFlag IncompleteThe
includeArchivedflag is still present in several files, includingresolver_team.goandgenerated.go. This indicates that the flag has not been fully removed from theGetProjectsGraphQL query as intended in the PR objectives.🔗 Analysis chain
Line range hint
1-724: Verify removal ofincludeArchivedflagThe changes implementing the deletion flag for projects look good. However, I couldn't find any changes related to removing the
includeArchivedflag from theGetProjectsGraphQL query as mentioned in the PR objectives.Please ensure that the
includeArchivedflag has been removed from the relevant GraphQL query definition, which is likely in a different file. You can use the following script to locate and verify the changes:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate GraphQL query definitions and check for includeArchived flag removal # Search for GraphQL query definitions containing 'GetProjects' echo "Searching for GetProjects GraphQL query definition:" rg --type go "type.*GetProjects.*{" -A 10 # Search for occurrences of 'includeArchived' in Go files echo "\nSearching for 'includeArchived' occurrences:" rg --type go "includeArchived"Length of output: 1986
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
1177-1177: LGTM! Consider adding a comment for the new field.The addition of the
IsDeletedfield is correct and aligns with the PR objectives. The field is properly typed and tagged.Consider adding a comment to explain the purpose of this field, for example:
// IsDeleted indicates whether the project has been marked as deleted. IsDeleted bool `json:"isDeleted"`
1873-1873: LGTM! Consider adding a comment for the new field.The addition of the
Deletedfield in theUpdateProjectInputstruct is correct and aligns with the PR objectives. The field is properly typed as*booland tagged withomitempty.Consider adding a comment to explain the purpose of this field, for example:
// Deleted, if set, updates the deletion status of the project. Deleted *bool `json:"deleted,omitempty"`server/internal/adapter/gql/generated.go (1)
Line range hint
62283-62288: Field Ordering inUpdateProjectInputInitializationThe field
deletedis added to theUpdateProjectInputstruct. Ensure that the fields are ordered logically, possibly placingdeletednear other status-related fields likearchivedfor better readability.server/gql/project.graphql (1)
148-148: Improve grammar in the commentThe comment
# not included deleted projectscould be clearer. Consider revising it to# does not include deleted projectsfor better readability.Apply this diff to correct the comment:
-): ProjectConnection! # not included deleted projects +): ProjectConnection! # does not include deleted projects
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (17)
- server/Makefile (1 hunks)
- server/e2e/gql_project_test.go (1 hunks)
- server/go.mod (1 hunks)
- server/gql/project.graphql (3 hunks)
- server/internal/adapter/gql/generated.go (22 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (2 hunks)
- server/internal/adapter/gql/loader_project.go (1 hunks)
- server/internal/adapter/gql/resolver_mutation_project.go (1 hunks)
- server/internal/adapter/gql/resolver_query.go (2 hunks)
- server/internal/infrastructure/memory/project.go (1 hunks)
- server/internal/infrastructure/mongo/mongodoc/project.go (2 hunks)
- server/internal/infrastructure/mongo/project.go (1 hunks)
- server/internal/usecase/interactor/project.go (2 hunks)
- server/internal/usecase/interactor/project_test.go (1 hunks)
- server/internal/usecase/interfaces/project.go (2 hunks)
- server/internal/usecase/repo/project.go (1 hunks)
- server/pkg/project/project.go (3 hunks)
🔇 Additional comments (27)
server/internal/infrastructure/mongo/mongodoc/project.go (3)
38-38: LGTM: Addition ofDeletedfieldThe new
Deletedboolean field in theProjectDocumentstruct aligns well with the PR objective of introducing a deletion flag to the project management system. The field type and name are appropriate for its purpose.
81-81: LGTM: Initialization ofDeletedfield inNewProjectThe
Deletedfield is correctly initialized in theNewProjectfunction usingproject.IsDeleted(). This approach is consistent with the initialization of other fields and leverages existing methods in theProjectmodel.
Line range hint
1-134: Summary of reviewThe changes in this file generally align with the PR objectives of introducing a deletion flag to the project management system. However, there are a couple of points that need attention:
- The
Modelmethod needs to be updated to include theDeletedstatus when constructing theproject.Projectinstance.- The removal of the
includeArchivedflag needs to be verified across the codebase.Please address these points to ensure consistency and completeness of the implementation.
server/internal/adapter/gql/loader_project.go (1)
Line range hint
1-125: Verify the removal of theincludeArchivedflag.The PR objectives mention removing the
includeArchivedflag from the GetProjects GraphQL query. However, this change is not visible in the current file. To ensure all PR objectives are met, we need to verify this change.Please run the following commands to check for the removal of the
includeArchivedflag:#!/bin/bash # Search for the GetProjects query definition echo "Searching for GetProjects query definition:" rg --type go 'type.*GetProjectsInput' server/internal/adapter/gql # Search for any remaining usage of includeArchived echo "Searching for any remaining usage of includeArchived:" rg --type go 'includeArchived' server/internal/adapter/gqlThis will help us confirm that the
includeArchivedflag has been removed from the GetProjects query and that there are no remaining usages of this flag in the GraphQL layer.server/pkg/project/project.go (2)
41-41: LGTM: Addition ofisDeletedfieldThe
isDeletedboolean field has been correctly added to theProjectstruct. This addition aligns with the PR objectives and follows Go conventions for struct field placement.
145-147: LGTM: Addition ofIsDeleted()methodThe
IsDeleted()method has been correctly implemented as a getter for theisDeletedfield. It follows Go conventions and maintains proper encapsulation.server/internal/infrastructure/mongo/project.go (1)
Line range hint
1-123: Verify implementation ofdeletedflag and permanent deletionWhile the new
FindDeletedByWorkspacemethod correctly uses thedeletedflag, I couldn't find where this flag is set or how permanent deletion is handled. To ensure complete implementation of the PR objectives:
- Verify that there's a method to set the
deletedflag (e.g., in theSavemethod or a newMarkAsDeletedmethod).- Confirm that the
Removemethod performs permanent deletion, as mentioned in the PR objectives for thedeleteProjectmutation.To check for the implementation of setting the
deletedflag and permanent deletion, run the following script:Please review the results to ensure that the
deletedflag can be set and that permanent deletion is handled correctly.server/internal/adapter/gql/resolver_mutation_project.go (1)
82-82: LGTM! Verify removal ofincludeArchived.The addition of the
Deletedparameter to theUpdateProjectParamstruct aligns with the PR objectives of introducing a deletion flag. This change looks good and maintains the existing functionality of theUpdateProjectmethod.However, the PR objectives also mention the removal of the
includeArchivedflag from the GetProjects GraphQL query. While this change is not visible in this file, it's important to ensure that it has been implemented correctly elsewhere in the codebase.To verify the removal of
includeArchived, please run the following script:This script will help confirm that
includeArchivedhas been removed from the relevant parts of the codebase.server/internal/adapter/gql/resolver_query.go (2)
263-265: LGTM: New DeletedProjects method added, but consider access control and testing.The addition of the
DeletedProjectsmethod aligns well with the PR objectives to introduce a "recycle bin" feature for projects. The implementation looks correct and consistent with other similar methods in the file.However, please consider the following suggestions:
Ensure proper access control checks are in place to prevent unauthorized access to deleted projects. You might want to verify if the user has the necessary permissions to view deleted projects for the given team.
Add unit tests for this new functionality to ensure it behaves as expected under various scenarios (e.g., when there are no deleted projects, when there are multiple deleted projects, etc.).
To verify the access control implementation, you can run the following command to check for any existing access control patterns in the codebase:
If this command returns results, consider implementing similar access control checks in the
DeletedProjectsmethod.Would you like assistance in generating unit tests for the
DeletedProjectsfunctionality?
239-241: LGTM: Simplified method signature, but verify impact.The removal of the
includeArchivedparameter from theProjectsmethod aligns with the PR objectives and simplifies the method signature. This is a good change.However, please ensure the following:
- Verify that this change doesn't break existing API consumers who might be passing the
includeArchivedparameter.- Update any relevant API documentation to reflect this change.
- Consider adding a deprecation notice in the GraphQL schema if this is a breaking change, to give API consumers time to adapt.
To verify the impact of this change, you can run the following command to check for any remaining usage of
includeArchivedin GraphQL queries:If this command returns any results, those areas of the codebase will need to be updated.
✅ Verification successful
Verification Successful: No Usage of
includeArchivedFoundThe search for
includeArchivedin GraphQL queries returned no results. This indicates that removing theincludeArchivedparameter from theProjectsmethod does not impact existing API consumers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GraphQL queries using includeArchived rg --type typescript --type javascript 'query.*Projects.*includeArchived'Length of output: 110
Script:
#!/bin/bash # Search for GraphQL queries using includeArchived in TypeScript and JavaScript files rg --type ts --type js 'query.*Projects.*includeArchived'Length of output: 59
server/go.mod (1)
26-26: LGTM: Addition of ULID library as a direct dependency.The addition of
github.com/oklog/ulid v1.3.1as a direct dependency is appropriate. This library provides functionality for generating and working with ULIDs (Universally Unique Lexicographically Sortable Identifiers), which can be useful for creating unique, sortable identifiers for projects or other entities in the system.To ensure this dependency is being used correctly, please run the following script to check its usage in the codebase:
✅ Verification successful
**** The ULID library
github.com/oklog/ulid v1.3.1is actively used in the codebase, confirming that its addition as a direct dependency is appropriate and necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added ULID library in the codebase. # Test: Search for import statements and function calls using the ULID library echo "Searching for ULID library usage:" rg --type go 'github.com/oklog/ulid' -A 5Length of output: 759
Script:
#!/bin/bash # Description: Check for ULID function calls and variable usages in resolver_mutation_project.go echo "Searching for ULID function calls and variable usages in resolver_mutation_project.go:" rg --type go 'ulid\.' server/internal/adapter/gql/resolver_mutation_project.goLength of output: 391
server/e2e/gql_project_test.go (2)
458-469: LGTM: Test setup looks goodThe test setup is well-structured. It initializes the server with appropriate configurations and creates three test projects. This provides a good foundation for testing the deletion functionality.
471-501: Verify thedeletedfield in the mutation responseThe mutation to mark a project as deleted is correctly implemented. However, the response structure in the GraphQL query doesn't include the
deletedfield. Instead, it usesisDeleted.To ensure consistency, please run the following script to check if
isDeletedis the correct field name used in the Project type:If the script confirms that
isDeletedis the correct field name, update the mutation query to include this field in the response:Query: `mutation UpdateProject($input: UpdateProjectInput!) { updateProject(input: $input) { project { id name - isDeleted + deleted updatedAt __typename } __typename } }`,server/internal/adapter/gql/gqlmodel/models_gen.go (1)
Line range hint
1-2453: Overall, the changes implement the deletion flag as intended.The additions to the
ProjectandUpdateProjectInputstructs correctly implement the new "deleted" flag functionality as described in the PR objectives. These changes allow for marking projects as deleted and updating their deletion status.To ensure these changes are properly utilized, let's check for any usage of these new fields:
server/internal/adapter/gql/generated.go (12)
846-846: Addition ofIsDeletedComplexity Function Looks GoodThe
IsDeletedfield is correctly added to handle complexity calculations for theisDeletedfield in theProjecttype.
6586-6597: Correct Complexity Handling forDeletedProjectsQueryThe complexity calculation for the
DeletedProjectsquery is implemented. Confirm that it correctly accounts forchildComplexityand any arguments to ensure accurate complexity estimation.
9601-9601: Inclusion ofdeletedField inUpdateProjectInputThe input field
deleted: Booleanis added toUpdateProjectInput. This allows clients to update the deletion status of a project. Ensure that the server-side update logic correctly handles this new field.
12642-12656: Arguments Parsing forDeletedProjectsQueryThe argument parser
field_Query_deletedProjects_argscorrectly processes theteamIdargument. Ensure that this method aligns with the expected GraphQL arguments to prevent issues during query execution.
39878-39921: Resolver forProject.isDeletedField ImplementedThe resolver function
_Project_isDeletedis correctly implemented to return theIsDeletedfield of aProjectobject. This ensures that theisDeletedfield is accessible in GraphQL queries.
71187-71191: Handling ofisDeletedin Field Marshal FunctionThe
isDeletedcase is added to the field marshal function to correctly serialize theisDeletedfield when returningProjectobjects. Verify that this serialization works as expected and that the field is correctly included in the GraphQL response.
73089-73110: Concurrency Handling fordeletedProjectsFieldIn the resolver map, the
deletedProjectsfield is correctly set up to handle concurrent execution. Ensure that this aligns with how other fields are handled and that there are no race conditions or data inconsistencies.
9665-9668:deletedProjectsQuery Added;includeArchivedFlag RemovedThe
deletedProjectsquery is introduced, and theincludeArchivedflag is removed from theprojectsquery. Verify that clients are updated to usedeletedProjectswhere necessary and that the removal ofincludeArchiveddoesn't affect existing functionality.Run this script to identify client-side usages:
#!/bin/bash # Description: Find usages of 'includeArchived' in client code. rg --type js,ts 'includeArchived' web/
6675-6675: Removal ofincludeArchivedfromProjectsComplexity FunctionThe complexity function for
Projectshas been updated to remove theincludeArchivedargument. Verify that this change is consistent throughout the complexity calculations and that no residual references remain.Use the following script to check for any remaining references:
#!/bin/bash # Description: Search for any remaining 'includeArchived' references. rg --type go 'includeArchived' server/
5755-5761: Verify Complexity Function forProject.isDeletedThe complexity calculation for
Project.isDeletedis added. Ensure that the functione.complexity.Project.IsDeletedis properly defined and returns the expected complexity value.Check the definition of the complexity function:
#!/bin/bash # Description: Verify definition of IsDeleted complexity function. rg --type go 'IsDeleted\s*func' server/internal/adapter/gql/generated.go
12762-12788: Removal ofincludeArchivedinProjectsArguments ParsingThe
includeArchivedargument is removed from theProjectsquery arguments parsing. Confirm that this change is consistently applied and that no residual code attempts to accessincludeArchived.Run the following script:
#!/bin/bash # Description: Ensure 'includeArchived' is not used in argument parsing. rg --type go 'includeArchived' server/internal/adapter/gql/
1013-1020: Ensure Proper Complexity Calculation for New QueriesThe addition of the
DeletedProjectscomplexity function and the updatedProjectsfunction reflect the schema changes. Please verify that the complexity estimations for these functions are accurate to prevent performance issues.Run the following script to check for any discrepancies in complexity calculations:
server/gql/project.graphql (1)
84-84: Approved: Addition ofdeletedfield toUpdateProjectInputThe inclusion of the
deletedfield inUpdateProjectInputappropriately aligns with the newisDeletedflag, allowing for updates to a project's deletion status.
server/Makefile
Outdated
| @@ -20,6 +20,7 @@ lint: | |||
| test: | |||
| go test -race -short -v ./... | |||
|
|
|||
| # make failcheck TEST_DIR=./internal/usecase/interactor | |||
There was a problem hiding this comment.
Please remove all of the unnecessary files.
server/go.mod
Outdated
| @@ -23,6 +23,7 @@ require ( | |||
| github.com/kennygrant/sanitize v1.2.4 | |||
| github.com/labstack/echo/v4 v4.11.4 | |||
| github.com/mitchellh/mapstructure v1.5.0 | |||
| github.com/oklog/ulid v1.3.1 | |||
There was a problem hiding this comment.
why is this direct dependency now?
There was a problem hiding this comment.
Okay, I'll put it back.
For some reason, in my environment, it happens when I run make gql.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
server/e2e/gql_project_test.go (3)
456-531: LGTM! Consider expanding test coverage.The
TestDeleteProjectsfunction effectively tests the basic functionality of marking a project as deleted and retrieving deleted projects. This aligns well with the PR objectives.To further improve the test coverage, consider adding the following scenarios:
- Verify that non-deleted projects are not returned in the
deletedProjectsquery.- Test the ability to undelete a project (if supported).
- Ensure that deleted projects are not returned in the regular
projectsquery.Would you like me to provide example code for these additional test scenarios?
454-454: Remove unnecessary commentThis commented line appears to be a command for running the test. It's generally good practice to keep the code free of commented-out code or commands. Consider removing this line to maintain code cleanliness.
- // go test -v -run TestDeleteProjects ./e2e/...
Removal of
includeArchivedFlag IncompleteThe
includeArchivedflag is still present in several.gofiles, specifically inserver/internal/adapter/gql/resolver_team.goandserver/internal/adapter/gql/generated.go. Please ensure that the flag is fully removed from all relevant parts of the codebase to meet the PR objectives.🔗 Analysis chain
Line range hint
1-531: Verify removal ofincludeArchivedflagThe PR objectives mention removing the
includeArchivedflag from the GetProjects GraphQL query. However, this change is not visible in the current file. Please ensure that this removal has been implemented in the appropriate files.To confirm the removal of the
includeArchivedflag, you can run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of 'includeArchived' in GraphQL queries and schema # Search for 'includeArchived' in .graphql files echo "Searching for 'includeArchived' in .graphql files:" rg --type graphql 'includeArchived' # Search for 'includeArchived' in Go files echo "Searching for 'includeArchived' in Go files:" rg --type go 'includeArchived' # If 'includeArchived' is not found, it has likely been removed as intendedLength of output: 1906
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- server/internal/adapter/gql/generated.go (22 hunks)
🔇 Additional comments (18)
server/internal/adapter/gql/generated.go (18)
846-846: LGTM: Addition ofIsDeletedcomplexity calculationThe
IsDeletedfield has been correctly added to theComplexityRoottype for theProjectobject. This addition aligns with the PR objective of introducing adeletedflag to the project management system.
1013-1013: LGTM: Addition ofDeletedProjectscomplexity calculationThe
DeletedProjectsfield has been correctly added to theComplexityRoottype for theQueryobject. This addition aligns with the PR objective of introducing a new GraphQL query for retrieving deleted projects.
5755-5761: LGTM: Added complexity calculation forProject.isDeletedThe complexity calculation for the
Project.isDeletedfield has been correctly implemented. It follows the same pattern as other fields in theProjecttype and aligns with the PR objective of introducing adeletedflag to the project management system.
6586-6597: LGTM: Added complexity calculation forQuery.deletedProjectsThe complexity calculation for the
Query.deletedProjectsfield has been correctly implemented. It follows the same pattern as other query fields and aligns with the PR objective of introducing a new GraphQL query for retrieving deleted projects.
6675-6675: LGTM: Updated complexity calculation forQuery.projectsThe complexity calculation for the
Query.projectsfield has been correctly updated to remove theincludeArchivedparameter. This change aligns with the PR objective of removing theincludeArchivedflag from the GetProjects GraphQL query.
9601-9601: LGTM: Addeddeletedfield toUpdateProjectInputtypeThe
deleted: Booleanfield has been correctly added to theUpdateProjectInputtype. This addition aligns with the PR objective of allowing users to modify the deletion flag using theUpdateProjectmutation. The nullable nature of the field (Boolean instead of Boolean!) allows for partial updates without necessarily changing the deletion status.
9668-9668: LGTM: AddeddeletedProjectsquery toQuerytypeThe
deletedProjects(teamId: ID!): ProjectConnection!query has been correctly added to theQuerytype. This addition aligns with the PR objective of introducing a new GraphQL query for retrieving deleted projects. The query structure is appropriate, taking ateamIdparameter and returning aProjectConnectionfor paginated results.
12642-12656: LGTM: Added argument parsing fordeletedProjectsqueryThe argument parsing logic for the
deletedProjectsquery has been correctly implemented. It follows the same pattern as other queries in the file and properly handles theteamIdargument of typegqlmodel.ID.
12762-12788: LGTM: Updated argument parsing forprojectsqueryThe argument parsing for the
projectsquery has been correctly updated to remove theincludeArchivedparameter. This change aligns with the PR objective of removing theincludeArchivedflag from the GetProjects GraphQL query. The parsing for the remaining arguments (pagination, keyword, and sort) has been properly maintained.
39878-39920: LGTM: Added_Project_isDeletedresolverThe
_Project_isDeletedresolver has been correctly implemented for theisDeletedfield of theProjecttype. It follows the same pattern as other field resolvers in the file and properly handles potential errors and null cases.
40149-40150: LGTM: UpdatedProjecttype field context functionsThe
isDeletedfield has been correctly added to the field context functions for theProjecttype in multiple locations. These consistent additions ensure that theisDeletedfield is properly handled in various GraphQL operations.Also applies to: 40386-40387, 40484-40485
45237-45237: LGTM: UpdatedProjectsquery resolverThe
Projectsquery resolver has been correctly updated to remove theincludeArchivedparameter. This change aligns with the PR objective of removing theincludeArchivedflag from the GetProjects GraphQL query. The resolver now uses the updated method signature correctly.
45414-45477: LGTM: AddedDeletedProjectsquery resolverThe
_Query_deletedProjectsresolver has been correctly implemented for thedeletedProjectsquery. It follows the same pattern as other query resolvers in the file, properly handles potential errors and null cases, and uses the appropriate method from theQueryinterface.
47875-47876: LGTM: UpdatedProjecttype field resolversThe
isDeletedfield has been correctly added to the field resolvers for theProjecttype. This addition ensures that theisDeletedfield is properly resolved in GraphQL operations and follows the same pattern as other field resolvers in theProjecttype.
62283-62283: LGTM: UpdatedUpdateProjectInputtypeThe
deletedfield has been correctly added to theUpdateProjectInputtype:
- It has been included in the list of fields in order.
- The unmarshaling logic for the input type has been updated to handle the
deletedfield.These changes align with the PR objective of allowing users to modify the deletion flag using the
UpdateProjectmutation. The unmarshaling logic correctly handles the nullable nature of thedeletedfield.Also applies to: 62423-62429
71187-71191: LGTM: UpdatedProjecttype field selectionThe
isDeletedfield has been correctly added to the field selection logic for theProjecttype. This addition ensures that theisDeletedfield can be properly selected in GraphQL queries and follows the same pattern as other fields in theProjecttype.
73089-73110: LGTM: AddeddeletedProjectsquery to root resolverThe
deletedProjectsquery has been correctly added to the root resolver logic. This addition ensures that thedeletedProjectsquery can be executed at the root level of GraphQL operations and follows the same pattern as other root-level queries.
9544-9544: LGTM: AddedisDeletedfield toProjecttypeThe
isDeleted: Boolean!field has been correctly added to theProjecttype. This addition aligns with the PR objective of introducing adeletedflag to the project management system.To ensure that the
isDeletedfield is properly implemented and populated, please run the following verification script:This script will help us verify that the
IsDeletedresolver is properly implemented for theProjecttype.✅ Verification successful
Verified:
isDeletedfield implementation confirmedThe
IsDeletedresolver for theProjecttype is properly implemented in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the implementation of the isDeleted field in the Project resolver rg --type go 'func \(.*\) IsDeleted\(' server/Length of output: 115
| Projects(ctx context.Context, teamID gqlmodel.ID, pagination *gqlmodel.Pagination, keyword *string, sort *gqlmodel.ProjectSort) (*gqlmodel.ProjectConnection, error) | ||
| CheckProjectAlias(ctx context.Context, alias string) (*gqlmodel.ProjectAliasAvailability, error) | ||
| StarredProjects(ctx context.Context, teamID gqlmodel.ID) (*gqlmodel.ProjectConnection, error) | ||
| DeletedProjects(ctx context.Context, teamID gqlmodel.ID) (*gqlmodel.ProjectConnection, error) |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: includeArchived Still Present in GraphQL Schema
The includeArchived parameter is still found in the GraphQL schema:
server/gql/workspace.graphql:projects(includeArchived: Boolean, first: Int, last: Int, after: Cursor, before: Cursor): ProjectConnection!
Please remove or update this parameter to ensure consistency with the QueryResolver interface changes.
🔗 Analysis chain
LGTM: Updated Projects method and added DeletedProjects method
The changes to the QueryResolver interface are consistent with the PR objectives:
- The
includeArchivedparameter has been removed from theProjectsmethod. - A new
DeletedProjectsmethod has been added.
These changes align with the removal of the includeArchived flag and the introduction of a new query for retrieving deleted projects.
To ensure that the removal of includeArchived doesn't break existing functionality, please run the following verification script:
If this script returns any results, we should review those occurrences to ensure they are updated or removed as necessary.
Let's rerun the verification to accurately check for any remaining usage of includeArchived in GraphQL queries or mutations.
Let's update the verification script to correctly search for includeArchived in GraphQL files by specifying the appropriate file extensions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of 'includeArchived' in GraphQL queries or mutations
rg --type graphql 'includeArchived'
Length of output: 115
Script:
#!/bin/bash
# Description: Check for any remaining usage of 'includeArchived' in GraphQL queries or mutations
rg --type graphql 'includeArchived'
Length of output: 67
Script:
#!/bin/bash
# Description: Check for any remaining usage of 'includeArchived' in GraphQL queries or mutations
rg 'includeArchived' --glob '*.graphql' --glob '*.gql'
Length of output: 194
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1169 +/- ##
=======================================
Coverage 13.85% 13.85%
=======================================
Files 595 595
Lines 64056 64062 +6
Branches 660 660
=======================================
+ Hits 8873 8877 +4
- Misses 55183 55185 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Overview
Add
deletedflag to projectWhat I've done
Deletion flag added to Project.
To update the delete flag, please use the
UpdateProjectmutation.Please note that the existing deleteProject mutation physically deletes the project from the database, so caution is required.
I have added
DeletedProjectsto the GraphQL query.This retrieves all entries where the delete flag is set to true.
I have removed the
includeArchivedflag from the GetProjects GraphQL query.While the frontend currently does not make queries using includeArchived, the backend will no longer support it.
What I haven't done
How I tested
Testing with Test Code
Which point I want you to review particularly
Please remove
includeArchivedfrom the web code.The update for the delete flag will be similar to the usage of
ARCHIVE_PROJECT.DeletedProjectswill be used similarly toStarredProjects.Memo
https://www.notion.so/eukarya/BE-Projects-Recycle-bin-82ca047f781740febd354db58aa2acea
Summary by CodeRabbit
Release Notes
New Features
failchecktarget for unit tests.Bug Fixes
Tests
These updates enhance user experience by providing better project management tools and ensuring data integrity.