refactored getting documents from subspace to use lookup #387
Conversation
WalkthroughThis pull request introduces a new GraphQL query named Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/functional-api/storage/auth/private-space-private-ch-document-auth.it-spec.ts (1)
6-6: Architectural improvement: Simplified document access patternThe refactoring successfully:
- Simplifies the API by using
profileIdinstead of separatespaceIdandchallengeId- Implements a consistent lookup-based data access pattern
- Maintains proper security contexts for both public and private challenges
- Reduces coupling by removing the need to know both space and challenge IDs
This change improves the code's maintainability and follows the principle of least knowledge.
Also applies to: 115-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
test/generated/alkemio-schema.tsis excluded by!**/generated/**test/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
graphql/queries/journeys/challenge/getChallengeDocumentAndStorageData.graphql(0 hunks)graphql/queries/journeys/challenge/getProfileDocumentAndStorageData.graphql(1 hunks)test/functional-api/roleset/roles-request.params.ts(1 hunks)test/functional-api/storage/auth/private-space-private-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/private-space-public-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/public-space-private-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/public-space-public-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/upload.params.ts(2 hunks)
💤 Files with no reviewable changes (1)
- graphql/queries/journeys/challenge/getChallengeDocumentAndStorageData.graphql
✅ Files skipped from review due to trivial changes (1)
- test/functional-api/roleset/roles-request.params.ts
🔇 Additional comments (26)
test/functional-api/storage/auth/public-space-private-ch-document-auth.it-spec.ts (12)
6-6: Import getProfileDocuments function
The getProfileDocuments function is correctly imported for use in this test file.
115-116: Update function call to getProfileDocuments
The function call has been updated to use getProfileDocuments with the correct profileId parameter, aligning with the new query structure.
121-121: Verify the data path in the response handling
Ensure that the data path getDocId.data?.lookup?.profile?.storageBucket?.documents[0].id matches the structure returned by the GetProfileDocuments query to prevent potential undefined access errors.
137-138: Update function call within parameterized tests
The function calls within the parameterized tests have been correctly updated to use getProfileDocuments with the appropriate profileId and userRole parameters.
142-142: Verify the data path in the assertion
Ensure that the data extraction res.data?.lookup?.profile?.storageBucket?.documents[0] correctly corresponds to the updated response structure from the GetProfileDocuments query.
169-173: Update storage bucket access in tests
The storage bucket access within the test cases has been correctly updated to reflect the new profile-based data structure.
198-199: Update function call to retrieve profile documents
The function call to getProfileDocuments is correctly updated with the profileId, ensuring consistency across test cases.
219-220: Update function call in parameterized tests
The function call has been appropriately updated to use the new getProfileDocuments function with the correct parameters.
251-252: Consistent use of getProfileDocuments function
The getProfileDocuments function is consistently used throughout the test cases, ensuring reliability in the tests.
272-273: Retrieve storage bucket ID using updated function
The retrieval of the storage bucket ID now correctly uses getProfileDocuments, which aligns with the refactored query structure.
285-286: Update function call for document retrieval
The function call to getProfileDocuments is correctly updated, maintaining consistency in the test setup.
307-308: Verify data path after response
Ensure that res.data?.lookup.profile?.storageBucket?.documents[0] correctly matches the updated response structure from the new query.
graphql/queries/journeys/challenge/getProfileDocumentAndStorageData.graphql (1)
1-41: Add new GetProfileDocuments GraphQL query
The new GetProfileDocuments query is well-structured and retrieves the necessary profile information, including storage buckets, documents, visuals, and authorization details. This aligns with the refactored approach to streamline data retrieval.
test/functional-api/storage/upload.params.ts (2)
74-74: Simplify deleteDocument function signature
The deleteDocument function parameters have been consolidated into a single line, enhancing readability without impacting functionality.
163-172: Refactor getProfileDocuments function
The getProfileDocuments function has been updated to use the new GetProfileDocuments GraphQL query, accepting profileID as a parameter. This refactor aligns with the updated query structure and ensures that document retrieval is based on profiles.
test/functional-api/storage/auth/public-space-public-ch-document-auth.it-spec.ts (7)
6-6: Import getProfileDocuments into test file
The getProfileDocuments function is correctly imported, allowing the test cases to utilize the updated document retrieval method.
115-116: Update function call to getProfileDocuments
The function call has been updated to use getProfileDocuments with the correct profileId, ensuring the test setup reflects the refactored code.
121-121: Verify data path consistency
Ensure that getDocId.data?.lookup?.profile?.storageBucket?.documents[0].id corresponds to the data structure returned by the updated GetProfileDocuments query.
137-138: Update test assertions with new function
The test assertions now correctly use getProfileDocuments and the appropriate parameters, maintaining the integrity of the test cases.
198-199: Consistent update of function calls
The function call to getProfileDocuments is correctly updated, ensuring the test reflects the changes in the document retrieval process.
219-220: Verify data extraction in tests
Check that the data path res.data?.lookup?.profile?.storageBucket?.documents[0] matches the updated response structure to prevent any data access issues.
169-170:
Potential inconsistency in entity ID
There might be an inconsistency with the entity ID used here. Please verify that entitiesId.space.profileId is the correct ID in this context, instead of entitiesId.challenge.profileId.
Apply this diff if necessary:
- entitiesId.space.profileId,
+ entitiesId.challenge.profileId,Likely invalid or redundant comment.
test/functional-api/storage/auth/private-space-public-ch-document-auth.it-spec.ts (2)
6-6: LGTM: Import statement updated correctly
The import statement has been updated to use the new getProfileDocuments function, aligning with the refactoring changes.
115-122: Verify function parameter changes
The refactoring from getChallengeProfileDocuments to getProfileDocuments has been implemented consistently across all test cases. The new function correctly uses profileId instead of separate spaceId and challengeId parameters.
Also applies to: 137-142, 169-174, 198-204, 219-226, 252-258, 273-280, 286-293, 308-315, 341-346
✅ Verification successful
Function refactoring has been consistently implemented
The verification confirms that:
- No instances of the old
getChallengeProfileDocumentsfunction remain in the codebase - All calls to
getProfileDocumentsconsistently use two parameters:profileId(accessed viaentitiesId.challenge.profileIdorentitiesId.space.profileId)- User role parameter (either
TestUser.GLOBAL_ADMINoruserRole)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of getProfileDocuments
# Test: Search for any remaining instances of getChallengeProfileDocuments
rg "getChallengeProfileDocuments"
# Test: Verify parameter usage in getProfileDocuments calls
rg "getProfileDocuments\(" -A 2
Length of output: 14568
test/functional-api/storage/auth/private-space-private-ch-document-auth.it-spec.ts (2)
6-6: LGTM: Import statement updated consistently
The import statement matches the changes in the public challenge test file, maintaining consistency across the test suite.
116-122: Verify test expectations for private challenge context
The refactoring maintains the correct security context for private challenges:
- Function calls updated consistently to use
getProfileDocuments - Data access paths correctly use the lookup pattern
- Test expectations properly reflect private challenge access rules (undefined privileges for non-members)
Also applies to: 138-142, 170-174, 199-204, 220-226, 253-258, 274-280, 287-293, 309-315, 342-346
test/functional-api/storage/auth/public-space-private-ch-document-auth.it-spec.ts
Show resolved
Hide resolved
test/functional-api/storage/auth/private-space-public-ch-document-auth.it-spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
test/functional-api/storage/auth/organization-document-auth.it-spec.ts (1)
86-86: Consider adding null check assertionsThe new data path
lookup.profileintroduces additional nesting levels. While the optional chaining handles null values gracefully, consider adding explicit assertions to verify the response structure.const data = res.data?.lookup?.profile?.storageBucket?.documents[0]; +expect(res.data?.lookup?.profile).toBeDefined(); +expect(res.data?.lookup?.profile?.storageBucket).toBeDefined(); const dataAuthorization = data?.authorization;Also applies to: 105-105, 135-135, 165-165, 184-184, 214-214, 236-236, 249-249, 268-268, 298-298
test/functional-api/storage/auth/private-space-document-auth.it-spec.ts (1)
Line range hint
1-1024: Comprehensive test coverage with consistent structureThe test file maintains excellent coverage across different document access scenarios:
- Space Profile visuals and references
- Link collections
- Post Card visuals and references
- Whiteboard documents
- Various user roles and their privileges
Each test suite follows a consistent pattern of:
- Setup with proper cleanup
- Document creation and retrieval
- Privilege and access verification
Consider adding test cases for:
- Error scenarios (e.g., invalid profile IDs)
- Edge cases in permission inheritance
- Concurrent access scenarios
test/functional-api/storage/auth/user-document-auth.it-spec.ts (2)
197-217: Consider reducing setup code duplicationThe setup code for retrieving storage and document IDs is repeated across test blocks. Consider extracting this into a helper function.
async function getStorageAndDocumentIds(profileId: string, userRole: TestUser) { const storageResponse = await getProfileDocuments(profileId, userRole); const storageId = storageResponse.data?.lookup?.profile?.storageBucket?.id ?? ''; await uploadFileOnStorageBucket( path.join(__dirname, 'files-to-upload', 'image.png'), storageId, TestUser.QA_USER ); const docResponse = await getProfileDocuments(profileId, userRole); const documentId = docResponse.data?.lookup?.profile?.storageBucket?.documents[0].id ?? ''; return { storageId, documentId }; }
Line range hint
1-265: Overall implementation looks solid with room for minor improvementsThe refactoring to use lookup for retrieving documents is consistently implemented throughout the test file. The test coverage remains comprehensive across different user roles and document types. Consider the suggested improvements for error handling and code organization, but these are not blocking issues.
For future improvements:
- Consider adding explicit error handling tests
- Extract common setup code into helper functions
- Add test cases for edge cases (e.g., missing documents, invalid profile IDs)
test/functional-api/storage/auth/public-space-document-auth.it-spec.ts (2)
Line range hint
1-800: Consider extracting common test data setupThe test file has repeated setup patterns across describe blocks. Consider extracting common setup logic into helper functions to improve maintainability.
Example refactor:
const setupTestDocument = async (config: { profileId: string, userRole: TestUser, filePath: string }) => { const getDocId = await getProfileDocuments( config.profileId, config.userRole ); return getDocId.data?.lookup?.profile?.storageBucket?.documents[0].id ?? ''; };
Based on the search results and the original test file content, I can now provide a final response:
Add error scenario test coverage for document authorization
The current test suite focuses on verifying authorization privileges for valid scenarios but lacks error handling tests. Consider adding test cases for:
- Invalid profile IDs (e.g., non-existent or malformed IDs)
- Missing storage buckets
- Missing or deleted documents
- Unauthorized access attempts
Example locations needing coverage:
getProfileDocuments()error scenariosuploadImageOnVisual()with invalid visual IDscreateReferenceOnProfile()with non-existent profilesThe only error test found was for file type validation in
uploads.it-spec.ts, indicating a gap in error scenario coverage for authorization flows.🔗 Analysis chain
Line range hint
1-800: Verify test coverage for error scenariosWhile the tests thoroughly cover the happy path and various user roles, consider adding test cases for:
- Invalid profile IDs
- Missing storage buckets
- Network failures during document retrieval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing error scenario tests rg -A 5 "test\(.*(error|invalid|fail|missing)" test/functional-api/storage/auth/Length of output: 82
Script:
#!/bin/bash # Let's try a broader search for error handling tests in storage-related files rg -A 5 "test\(.*(?i)(error|invalid|fail|missing)" test/functional-api/storage/ # Also check for error handling in the implementation rg -A 5 "(?i)(error|invalid|fail|missing)" src/domain/storage/Length of output: 707
Script:
#!/bin/bash # Let's check for error handling in the test file under review rg -A 5 "expect.*toThrow|catch" test/functional-api/storage/auth/public-space-document-auth.it-spec.ts # Let's also check the implementation files related to storage fd -e ts storageLength of output: 200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
test/generated/alkemio-schema.tsis excluded by!**/generated/**test/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (9)
graphql/queries/organization/getOrganizationDocumentAndStorageData.graphql(0 hunks)graphql/queries/user/getUserDocumentAndStorageData.graphql(0 hunks)test/functional-api/storage/auth/organization-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/private-space-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/private-space-private-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/public-space-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/public-space-public-ch-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/auth/user-document-auth.it-spec.ts(10 hunks)test/functional-api/storage/upload.params.ts(2 hunks)
💤 Files with no reviewable changes (2)
- graphql/queries/user/getUserDocumentAndStorageData.graphql
- graphql/queries/organization/getOrganizationDocumentAndStorageData.graphql
🚧 Files skipped from review as they are similar to previous changes (3)
- test/functional-api/storage/auth/public-space-public-ch-document-auth.it-spec.ts
- test/functional-api/storage/upload.params.ts
- test/functional-api/storage/auth/private-space-private-ch-document-auth.it-spec.ts
🔇 Additional comments (9)
test/functional-api/storage/auth/organization-document-auth.it-spec.ts (2)
6-6: LGTM: Import change aligns with refactoring strategy
The change to import getProfileDocuments aligns with the PR's objective to refactor document retrieval using lookup.
81-86: Verify test coverage remains comprehensive
The switch to using profileId maintains the test structure while adapting to the new API. However, let's verify that all original test scenarios are still covered.
Also applies to: 101-105
✅ Verification successful
Let me gather more information about the test coverage by examining the test structure and permissions.
Test coverage is comprehensive and correctly maintained
The verification shows that:
- All user roles (GLOBAL_ADMIN, HUB_ADMIN, CHALLENGE_ADMIN, HUB_MEMBER, NON_HUB_MEMBER) are being tested
- Permission levels are thoroughly tested for each role with appropriate privileges
- Test structure covers all scenarios: visual documents, reference documents, and description documents
- The switch to using
profileIdis consistently applied across all test cases while maintaining the same authorization testing patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all test scenarios are preserved after refactoring
# Check for any removed test cases by comparing with git history
git log -p -- test/functional-api/storage/auth/organization-document-auth.it-spec.ts | grep -B 5 -A 5 "test.each"
# Verify all user roles are still being tested
rg "TestUser\." test/functional-api/storage/auth/organization-document-auth.it-spec.ts
Length of output: 4003
Script:
#!/bin/bash
# Check the test structure and permissions testing
rg "test\.(each|it|describe)" test/functional-api/storage/auth/organization-document-auth.it-spec.ts -A 2
# Look for getProfileDocuments usage
rg "getProfileDocuments" test/functional-api/storage/auth/organization-document-auth.it-spec.ts -B 2 -A 2
Length of output: 3859
test/functional-api/storage/auth/private-space-document-auth.it-spec.ts (2)
6-6: LGTM: Import aligned with new profile-based document retrieval
The import change reflects the architectural shift from space-based to profile-based document retrieval.
99-104: Verify consistent usage of profile-based document retrieval
The test setup has been correctly updated to use profile IDs and the new lookup-based data structure. The changes are consistent with the architectural shift described in the PR.
Also applies to: 118-122
✅ Verification successful
Based on the search results, I can see that:
- The codebase consistently uses
getProfileDocumentsfor document retrieval - All test files follow the same pattern of accessing documents through
lookup.profile.storageBucket - There are no remaining instances of space-based document retrieval
- The changes are consistently applied across all test files in the storage/auth directory
Profile-based document retrieval changes verified successfully
The changes to use profile IDs and the new lookup-based data structure are consistently implemented across the test suite. All document retrievals use the getProfileDocuments function and access data through the lookup.profile.storageBucket path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of profile-based document retrieval across test files
# Test: Search for any remaining instances of space-based document retrieval
rg -g '*.ts' 'getSpaceProfileDocuments'
# Test: Verify the new profile-based approach is used consistently
rg -g '*.ts' 'lookup\.profile\.storageBucket'
Length of output: 87
Script:
#!/bin/bash
# Let's try a broader search to understand document retrieval patterns
# Search for document retrieval related functions
rg -g '*.ts' 'getProfileDocuments|getDocuments|storageBucket'
# Search for profile-based storage access patterns
rg -g '*.ts' 'profile.*storage|storage.*profile'
# Look for test files related to document storage
fd -e ts -e spec.ts -e test.ts storage
Length of output: 124068
test/functional-api/storage/auth/user-document-auth.it-spec.ts (3)
6-6: LGTM: Import change aligns with the new lookup-based implementation
The renamed import reflects the refactoring to use lookup for retrieving profile documents.
136-141: LGTM: Comprehensive test coverage for reference documents
The test setup and assertions properly verify document access through the new lookup-based implementation while maintaining good test coverage across different user roles.
63-69: Verify the lookup implementation's error handling
While the changes correctly implement the new lookup-based approach, we should ensure proper error handling for cases where the lookup fails.
Consider adding error handling tests:
test('should handle lookup failures gracefully', async () => {
const res = await getProfileDocuments('invalid-profile-id', TestUser.QA_USER);
expect(res.data?.lookup?.profile).toBeNull();
});✅ Verification successful
Based on the search results, I can see that the lookup implementation consistently uses the optional chaining operator (?.) and nullish coalescing operator (??) for handling potential null/undefined values. The code follows a pattern where:
- All lookup paths use optional chaining to safely access nested properties
- Default values are provided using nullish coalescing when extracting IDs and other data
- The tests verify the authorization and privileges even when accessing nested lookup paths
For example:
documentId = getDocId.data?.lookup?.profile?.storageBucket?.documents[0].id ?? '';
const data = res.data?.lookup?.profile?.storageBucket?.documents[0];The lookup implementation has proper error handling
The code already implements robust error handling through:
- Optional chaining to safely traverse the lookup path
- Nullish coalescing to provide fallback values
- Tests that verify authorization and privileges at each level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in lookup implementations
rg -A 3 "lookup\?" .
Length of output: 67562
test/functional-api/storage/auth/public-space-document-auth.it-spec.ts (2)
6-6: LGTM: Import change aligns with the new lookup-based approach
The change from a specific document getter to a more generic getProfileDocuments reflects the architectural shift to using lookups for document retrieval.
99-104: Consistent implementation of the new lookup-based document retrieval pattern
The changes consistently update the data access pattern across all test cases to use the new lookup-based structure:
- Changed from direct entity access to profile-based lookup
- Updated response path from
data?.space?.subspacetodata?.lookup?.profile - Maintained consistent parameter usage with
profileId
Also applies to: 118-122, 147-151, 176-181, 195-200, 225-230, 246-252, 259-265, 279-284, 309-313
Summary by CodeRabbit