Communication: Add option to toggle channel privacy
#10315
Conversation
asliayk
added
tests
client
github-actions
bot
added
server
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
WalkthroughThe pull request implements functionality for toggling a channel’s privacy status. On the backend, a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as ConversationSettingsComponent
participant CS as ChannelService
participant CR as ChannelResource (Server)
participant DB as Database
U->>UI: Click on toggle privacy button
UI->>CS: Call toggleChannelPrivacy(courseId, channelId)
CS->>CR: POST /{courseId}/channels/{channelId}/toggle-privacy
CR->>DB: Retrieve channel
DB-->>CR: Return channel data
CR->>DB: Update channel's privacy state
DB-->>CR: Return updated channel data
CR-->>CS: Respond with channel DTO
CS-->>UI: Return response
UI->>UI: Emit channelPrivacyChange event and update UI
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| ✨ Finishing Touches
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
|
![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
🧹 Nitpick comments (6)
src/test/javascript/spec/component/overview/course-conversations/services/channel.service.spec.ts (1)
205-217: Consider adding error scenario test cases.While the happy path test case is well-implemented, consider adding test cases for error scenarios:
- Network errors
- Server errors (4xx/5xx responses)
- Invalid response data
Example test case for error handling:
+ it('toggleChannelPrivacy should handle server error', fakeAsync(() => { + service + .toggleChannelPrivacy(1, 2) + .pipe(take(1)) + .subscribe({ + error: (error) => expect(error.status).toBe(500) + }); + + const req = httpMock.expectOne({ method: 'POST', url: '/api/courses/1/channels/2/toggle-privacy' }); + req.flush('Internal Server Error', { status: 500, statusText: 'Internal Server Error' }); + tick(); + }));src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (2)
187-205: Consider consolidating modal opening methods.The
openPublicChannelModalandopenPrivateChannelModalmethods have similar structures and could be consolidated into a single method with a parameter for the modal type.- openPublicChannelModal(channel: ChannelDTO) { - const keys = { - titleKey: 'artemisApp.dialogs.publicChannel.title', - questionKey: 'artemisApp.dialogs.publicChannel.question', - descriptionKey: 'artemisApp.dialogs.publicChannel.description', - confirmButtonKey: 'artemisApp.dialogs.publicChannel.confirmButton', - }; - this.openPrivacyChangeModal(channel, keys); - } - - openPrivateChannelModal(channel: ChannelDTO) { - const keys = { - titleKey: 'artemisApp.dialogs.privateChannel.title', - questionKey: 'artemisApp.dialogs.privateChannel.question', - descriptionKey: 'artemisApp.dialogs.privateChannel.description', - confirmButtonKey: 'artemisApp.dialogs.privateChannel.confirmButton', - }; - this.openPrivacyChangeModal(channel, keys); - } + private openPrivacyModal(channel: ChannelDTO, isPublic: boolean) { + const modalType = isPublic ? 'public' : 'private'; + const keys = { + titleKey: `artemisApp.dialogs.${modalType}Channel.title`, + questionKey: `artemisApp.dialogs.${modalType}Channel.question`, + descriptionKey: `artemisApp.dialogs.${modalType}Channel.description`, + confirmButtonKey: `artemisApp.dialogs.${modalType}Channel.confirmButton`, + }; + this.openPrivacyChangeModal(channel, keys); + }
207-224: Enhance error handling in privacy change modal.The error handling could be improved by adding specific error messages for common failure scenarios.
private openPrivacyChangeModal(channel: ChannelDTO, keys: { titleKey: string; questionKey: string; descriptionKey: string; confirmButtonKey: string }) { const modalRef = this.createModal(channel, keys); this.openModal(modalRef, () => { this.channelService .toggleChannelPrivacy(this.course().id!, channel.id!) .pipe(takeUntil(this.ngUnsubscribe)) .subscribe({ next: (res) => { const updatedChannel = res.body; if (updatedChannel) { this.conversationAsChannel = updatedChannel; this.channelPrivacyChange.emit(); } }, - error: (errorResponse: HttpErrorResponse) => onError(this.alertService, errorResponse), + error: (errorResponse: HttpErrorResponse) => { + const errorMessage = this.getPrivacyChangeErrorMessage(errorResponse); + onError(this.alertService, errorResponse, errorMessage); + }, }); }); } + + private getPrivacyChangeErrorMessage(error: HttpErrorResponse): string { + switch (error.status) { + case 403: + return 'You do not have permission to change the channel privacy'; + case 404: + return 'The channel could not be found'; + default: + return 'Failed to change channel privacy'; + } + }src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (2)
514-544: Enhance error handling and response entity in toggleChannelPrivacy.While the implementation is solid, consider these improvements:
- Add more specific error cases
- Use ResponseEntity builder pattern
- Add ETag support for caching
@PostMapping("{courseId}/channels/{channelId}/toggle-privacy") @EnforceAtLeastTutorInCourse public ResponseEntity<ChannelDTO> toggleChannelPrivacy(@PathVariable Long courseId, @PathVariable Long channelId) { log.debug("REST request to toggle privacy for channel : {}", channelId); checkCommunicationEnabledElseThrow(courseId); var channelFromDatabase = channelRepository.findByIdElseThrow(channelId); if (!channelFromDatabase.getCourse().getId().equals(courseId)) { throw new BadRequestAlertException("The channel does not belong to the course", CHANNEL_ENTITY_NAME, "channel.course.mismatch"); } var requestingUser = userRepository.getUserWithGroupsAndAuthorities(); channelAuthorizationService.isAllowedToUpdateChannel(channelFromDatabase, requestingUser); boolean isCurrentlyPublic = Boolean.TRUE.equals(channelFromDatabase.getIsPublic()); channelFromDatabase.setIsPublic(!isCurrentlyPublic); var updatedChannel = channelRepository.save(channelFromDatabase); + var channelDTO = conversationDTOService.convertChannelToDTO(requestingUser, updatedChannel); + String etag = "\"" + updatedChannel.getVersion() + "\""; - return ResponseEntity.ok(conversationDTOService.convertChannelToDTO(requestingUser, updatedChannel)); + return ResponseEntity + .ok() + .eTag(etag) + .body(channelDTO); }
524-526: Consider adding rate limiting for privacy toggle endpoint.To prevent abuse, consider adding rate limiting for the privacy toggle endpoint using Spring's
@RateLimiterannotation.src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
1013-1032: LGTM! Test method thoroughly validates channel privacy toggle functionality.The test method effectively:
- Verifies initial public state
- Tests toggling to private
- Tests toggling back to public
- Includes proper cleanup
However, consider adding these test scenarios for more comprehensive coverage:
- Test with insufficient permissions
- Test with archived channel
- Test with course-wide channel
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts(7 hunks)src/main/webapp/app/shared/metis/conversations/channel.service.ts(1 hunks)src/main/webapp/app/shared/metis/conversations/conversation-permissions.utils.ts(1 hunks)src/main/webapp/i18n/de/conversation.json(1 hunks)src/main/webapp/i18n/en/conversation.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts(7 hunks)src/test/javascript/spec/component/overview/course-conversations/services/channel.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.htmlsrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/metis/conversations/conversation-permissions.utils.tssrc/main/webapp/app/shared/metis/conversations/channel.service.tssrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.tssrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/overview/course-conversations/services/channel.service.spec.tssrc/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.tssrc/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/conversation.json
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (1)
67-69: LGTM!The
onPrivacyChange()method follows the established pattern and correctly handles privacy change events by marking changes and closing the dialog.src/main/webapp/app/shared/metis/conversations/conversation-permissions.utils.ts (1)
111-113: LGTM!The
canChangeChannelPrivacyState()function follows the established pattern of permission checks and correctly reuses thehasChannelModerationRights()helper function.src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
102-112: LGTM!The
toggleChannelPrivacy()method is well-documented and follows the established service patterns:
- Clear JSDoc documentation
- Consistent URL construction
- Proper error handling through Observable
- Reuse of date conversion pipeline
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)
131-165: LGTM! Comprehensive test coverage for dialog events.The test case thoroughly verifies the component's behavior for privacy, archival, deletion, and leave events. It checks both state changes and modal interactions, with proper spy usage and state resets between checks.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (1)
34-36: LGTM! Well-defined output event.The
channelPrivacyChangeoutput event follows Angular's event naming conventions.src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts (1)
178-236: LGTM! Thorough test coverage for privacy toggle functionality.The tests comprehensively verify:
- Channel privacy state changes
- Modal interactions for both public and private transitions
- Service call verification
- Proper use of fakeAsync for async operations
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (1)
75-75: LGTM! Event binding follows established patterns.The new
channelPrivacyChangeevent binding is properly integrated with other event handlers and follows Angular's event binding syntax.src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html (3)
16-28: LGTM! Privacy toggle button implementation is clear and consistent.The privacy toggle button:
- Uses appropriate icons for public/private states
- Includes clear text labels
- Follows Angular's template syntax
30-42: LGTM! Archival toggle button implementation is well-structured.The archival toggle button:
- Uses appropriate icons for archived/unarchived states
- Includes clear text labels
- Maintains consistency with the privacy toggle button
51-52: LGTM! Delete button improvements enhance usability.The delete button updates:
- Add
[renderButtonText]="false"for cleaner rendering- Include translated text in a separate span
Also applies to: 59-59
src/main/webapp/i18n/en/conversation.json (2)
105-116: LGTM! Dialog entries for privacy changes are comprehensive and clear.The new dialog entries:
- Include clear titles, questions, and descriptions
- Explain the implications of privacy changes
- Follow the established translation structure
127-131: LGTM! Action labels are descriptive and user-friendly.The updated action labels:
- Clarify the scope of archival actions ("for everyone")
- Use consistent wording for privacy changes
- Maintain clear and concise language
src/main/webapp/i18n/de/conversation.json (3)
111-116: LGTM! Public channel dialog translations are clear and consistent.The translations correctly use informal language ("du") and maintain consistency with other dialog translations in the file.
117-122: LGTM! Private channel dialog translations are clear and consistent.The translations correctly use informal language ("du") and maintain consistency with other dialog translations in the file.
127-131: LGTM! Conversation settings actions are clear and consistent.The translations correctly use consistent terminology and maintain alignment with the existing action translations.
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
this is because you should deploy this branch on a test server before testing:
as I see the branch is not deployed on ts3. |
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Checklist
General
Server
Client
Motivation and Context
Artemis currently lacks the ability to toggle channel privacy, a feature available in Slack's channel settings. Adding this option enables users to seamlessly switch between public and private channels, improving flexibility and access control.
Description
The ability to toggle channel privacy has been added. This feature is integrated into the Settings tab of the Conversation Details Dialog. It allows switching a public channel to private and vice versa. Informative confirmation dialogs have also been implemented.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
channel settings with new toggle channel privacy option
informative confirmation dialog
Summary by CodeRabbit