-
Notifications
You must be signed in to change notification settings - Fork 345
feat(content-sharing): Create sharing service changeSharedLinkPermission #4332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughI pity the fool who misses this: a new sharing service and Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ContentSharingV2
participant Hook as useSharingService
participant ItemAPI as File/Folder API
participant Service as sharingService
participant Modal as UnifiedShareModal
participant Utils as convertItemResponse
User->>ContentSharingV2: open share UI
ContentSharingV2->>Hook: init(api,item,id,type,setters)
Hook->>ItemAPI: select file or folder API
Hook->>Service: createSharingService(itemApiInstance,itemData,onSuccess)
ContentSharingV2->>Modal: render(..., sharingService)
note right of Modal: user changes shared link permission
Modal->>Service: changeSharedLinkPermission(level)
Service->>ItemAPI: updateSharedLink(itemData, permissions, onSuccess, {}, params)
ItemAPI-->>Service: success(updatedData)
Service->>Utils: convertItemResponse(updatedData)
Utils-->>Hook: convertedItem, convertedSharedLink
Hook->>ContentSharingV2: setItem(...), setSharedLink(...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
99-99: Duplicate issue, fool!I already flagged this permission duplication issue in the test file. The spread here creates both snake_case and camelCase versions of the same permission fields.
🧹 Nitpick comments (2)
src/elements/content-sharing/hooks/__tests__/useSharingService.test.ts (2)
92-112: Fool, this test ain't realistic enough!Line 108 passes
mockConvertedData(already converted) toonSuccessCallback, but in real usage, the callback receives raw API response data that then gets converted insidehandleSuccess. This test would catch more bugs if it used raw API data.Additionally, lines 110-111 only verify the setters were called once but don't check the actual arguments passed.
Consider this approach:
test('should handle success callback correctly', () => { + const mockRawApiData = { + id: mockItemId, + permissions: { can_download: false, can_preview: true }, + type: 'file', + // ... other raw API fields + }; const mockConvertedData = { item: { id: mockItemId, permissions: { can_download: false }, }, sharedLink: {}, }; - (convertItemResponse as jest.Mock).mockReturnValue(mockConvertedData); + (convertItemResponse as jest.Mock).mockReturnValueOnce(mockConvertedData); renderHook(() => useSharingService(mockApi, mockItem, mockItemId, TYPE_FILE, mockSetItem, mockSetSharedLink), ); // Get the onSuccess callback that was passed to mock createSharingService const onSuccessCallback = (createSharingService as jest.Mock).mock.calls[0][0].onSuccess; - onSuccessCallback(mockConvertedData); + onSuccessCallback(mockRawApiData); + expect(convertItemResponse).toHaveBeenCalledWith(mockRawApiData); - expect(mockSetItem).toHaveBeenCalledTimes(1); - expect(mockSetSharedLink).toHaveBeenCalledTimes(1); + expect(mockSetItem).toHaveBeenCalledWith(expect.any(Function)); + expect(mockSetSharedLink).toHaveBeenCalledWith(expect.any(Function)); });
115-137: Where's the success callback test, fool?The TYPE_FILE scenario has a success callback test (lines 92-112), but TYPE_FOLDER doesn't. You should test the success callback for folders too to ensure consistent coverage!
Add a test similar to lines 92-112:
test('should handle success callback correctly for folder', () => { const mockRawApiData = { id: mockItemId, permissions: { can_download: false, can_preview: true }, type: 'folder', // ... other raw API fields }; const mockConvertedData = { item: { id: mockItemId, permissions: { can_download: false }, }, sharedLink: {}, }; (convertItemResponse as jest.Mock).mockReturnValueOnce(mockConvertedData); renderHook(() => useSharingService(mockApi, mockItem, mockItemId, TYPE_FOLDER, mockSetItem, mockSetSharedLink), ); const onSuccessCallback = (createSharingService as jest.Mock).mock.calls[0][0].onSuccess; onSuccessCallback(mockRawApiData); expect(convertItemResponse).toHaveBeenCalledWith(mockRawApiData); expect(mockSetItem).toHaveBeenCalledWith(expect.any(Function)); expect(mockSetSharedLink).toHaveBeenCalledWith(expect.any(Function)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/elements/content-sharing/ContentSharingV2.tsx(3 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(3 hunks)src/elements/content-sharing/__tests__/sharingService.test.ts(1 hunks)src/elements/content-sharing/__tests__/useSharedLink.test.js(1 hunks)src/elements/content-sharing/hooks/__tests__/useSharingService.test.ts(1 hunks)src/elements/content-sharing/hooks/useSharingService.ts(1 hunks)src/elements/content-sharing/sharingService.ts(1 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/elements/content-sharing/__tests__/useSharedLink.test.js (2)
src/elements/content-sharing/hooks/useSharedLink.js (1)
changeSharedLinkPermissionLevel(39-42)src/elements/content-sharing/SharingModal.js (1)
changeSharedLinkPermissionLevel(87-90)
src/elements/content-sharing/__tests__/sharingService.test.ts (1)
src/elements/content-sharing/sharingService.ts (2)
convertSharedLinkPermissions(4-11)createSharingService(13-27)
src/elements/content-sharing/hooks/__tests__/useSharingService.test.ts (3)
src/elements/content-sharing/sharingService.ts (1)
createSharingService(13-27)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-106)src/elements/content-sharing/hooks/useSharingService.ts (1)
useSharingService(7-44)
src/elements/content-sharing/ContentSharingV2.tsx (2)
src/elements/content-sharing/hooks/useSharingService.ts (1)
useSharingService(7-44)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-106)
src/elements/content-sharing/hooks/useSharingService.ts (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-106)src/elements/content-sharing/sharingService.ts (1)
createSharingService(13-27)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/hooks/useSharingService.ts (1)
useSharingService(7-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (13)
src/elements/content-sharing/__tests__/useSharedLink.test.js (1)
39-40: Fool, this formatting looks good!I pity the fool who doesn't consolidate their useState declarations! This change makes the code more consistent with the pattern on lines 36-37.
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
50-52: Good setup, fool!I like this default mock setup! It properly initializes useSharingService to return null by default, which matches the hook behavior when item is not available.
src/elements/content-sharing/__tests__/sharingService.test.ts (1)
1-67: Excellent test coverage, fool!I pity the fool who doesn't write comprehensive tests! This test file covers:
- Permission mapping for both CAN_DOWNLOAD and CAN_PREVIEW
- Empty string handling
- Service creation and structure
- Correct parameter passing to updateSharedLink
Good work, sucka!
src/elements/content-sharing/ContentSharingV2.tsx (3)
11-11: Good integration, fool!The import and initialization of the useSharingService hook looks solid. The hook is properly called with all required dependencies (api, item, itemID, itemType, setItem, setSharedLink).
Also applies to: 52-52
57-65: Nice cleanup, sucka!I like this standardization from
...FromAPIto...FromApi. Consistent naming makes the code easier to read and maintain!
157-157: Proper wiring, fool!The sharingService is correctly passed to UnifiedShareModal. This enables the modal to trigger permission updates through the service.
src/elements/content-sharing/sharingService.ts (2)
4-11: Clean permission mapping, fool!This function correctly maps permission levels to boolean flags. The early return for falsy values prevents unnecessary object creation. Good work, sucka!
13-27: Solid service creation, fool!I like how this wraps the updateSharedLink call in a clean API. The changeSharedLinkPermission function properly converts the permission level and passes all required parameters to updateSharedLink.
src/elements/content-sharing/hooks/useSharingService.ts (2)
8-22: Proper API selection, fool!This memo correctly handles the API instance selection based on item type. Returns null when item is missing or itemType is unsupported. Dependencies are correct too!
24-41: Solid hook implementation, sucka!I like this sharingService memo! It:
- Properly guards against missing itemApiInstance
- Constructs itemData with necessary fields
- Sets up a clean handleSuccess callback that converts the response and atomically updates both item and sharedLink state
- Creates the service with all required dependencies
The dependencies array is correct, and the memoization prevents unnecessary service recreation.
src/elements/content-sharing/hooks/__tests__/useSharingService.test.ts (3)
1-32: Solid test setup, fool!The imports, mocks, and test data structure are well-organized. The mock setup correctly isolates the hook for unit testing.
35-45: Clean lifecycle management!The beforeEach and afterEach hooks properly manage mock state between tests.
47-67: I pity the fool who doesn't test edge cases!These tests properly verify the guard conditions when item is null or itemType is invalid. Good defensive testing!
src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/hooks/__tests__/useSharingService.test.ts
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
a683e6c to
aa3e67a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/hooks/useSharingService.ts (1)
useSharingService(7-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
54-63: I pity the fool who doesn't follow RTL patterns! This change is solid, fool!The refactor from
getWrappertorenderComponentwith a default empty object parameter addresses previous feedback perfectly. This is the proper React Testing Library pattern, sucka!
What
Before this PR: cannot update permission
After this PR: updateSharedLink API call (created and used by existing contentSharingV1) is called, the updated permissions are passed in successfully, and the UI reflex the item permission is updated properly.
Testing:
Screen.Recording.2025-10-09.at.3.30.21.PM.mov
Summary by CodeRabbit
New Features
Tests