Communication: Add course-wide channel option
#10052
Conversation
asliayk
added
tests
client
github-actions
bot
added
server
asliayk
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
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/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThis pull request introduces support for course-wide channels in the Artemis communication system. The changes span multiple components across frontend and backend, enabling users to create channels that automatically include all course participants. The implementation adds a new option during channel creation to specify whether a channel should be course-wide, with corresponding updates to form components, localization files, and backend resource handling. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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: 3
🧹 Nitpick comments (6)
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts (1)
56-59: Fix getter method nameThe getter method name has a typo:
isisCourseWideChannelControlshould beisCourseWideChannelControlto maintain consistency with other getter methods.- get isisCourseWideChannelControl() { + get isCourseWideChannelControl() { return this.form.get('isCourseWideChannel'); }src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.spec.ts (1)
77-77: Consider testing both true and false states.The test suite only verifies the
falsestate ofisCourseWideChannel. Add test cases for thetruestate to ensure both scenarios work correctly.Also applies to: 100-100
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts (1)
51-56: Consider enhancing initialization test.While the test verifies basic initialization, consider adding assertions for default values of all channel properties, including
isCourseWideChannel.src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.html (1)
68-92: Consider UX improvements for course-wide channel selection.The implementation is technically sound, but consider these UX improvements:
- Add a warning or confirmation when selecting course-wide channel, as it affects all course participants
- Consider disabling the public/private option when course-wide is selected, as course-wide channels might need to follow specific visibility rules
src/test/playwright/e2e/course/CourseMessages.spec.ts (1)
78-89: LGTM: Good coverage for course-wide channel creation, consider additional test cases.The test effectively verifies the basic functionality of creating course-wide channels. However, consider adding tests for:
- Creating private course-wide channels
- Verifying that all course participants are automatically added
- Testing permission restrictions (e.g., students shouldn't be able to create course-wide channels)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
235-235: Consider adding validation for course-wide channelsTo prevent potential misuse, consider adding validation to ensure that course-wide channels cannot also be announcement channels, as this combination might lead to notification spam for all course members.
channelToCreate.setIsCourseWide(channelDTO.getIsCourseWide()); +if (channelToCreate.getIsCourseWide() && channelToCreate.getIsAnnouncementChannel()) { + throw new BadRequestAlertException("Course-wide channels cannot be announcement channels", CHANNEL_ENTITY_NAME, "channelInvalidCombination"); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(1 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts(6 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.html(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts(2 hunks)src/main/webapp/i18n/de/conversation.json(2 hunks)src/main/webapp/i18n/en/conversation.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/AbstractConversationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java(6 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.spec.ts(3 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts(5 hunks)src/test/playwright/e2e/course/CourseMessages.spec.ts(1 hunks)src/test/playwright/support/pageobjects/course/CourseMessagesPage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/test/java/de/tum/cit/aet/artemis/communication/AbstractConversationTest.java (1)
Pattern 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/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.html (1)
Pattern 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/i18n/de/conversation.json (1)
Pattern 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/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.html (1)
Pattern 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/course-conversations.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
Pattern 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/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.spec.ts (1)
Pattern 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/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts (1)
Pattern 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/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
Pattern 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
🔇 Additional comments (20)
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.html (2)
7-7: LGTM: Translation key usage for course-wide channel title
The implementation correctly uses the translation pipe and conditional rendering for the course-wide channel title.
21-21: LGTM: Event binding for course-wide channel changes
The event binding follows Angular best practices and maintains consistency with existing channel type change handlers.
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (3)
22-23: LGTM: Property initialization for course-wide channel
The boolean property is correctly initialized with a default value of false.
32-35: LGTM: Event handler implementation
The handler follows the same pattern as other channel property handlers, maintaining consistency.
41-46: LGTM: Channel creation logic update
The implementation correctly handles the course-wide channel property during channel creation.
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts (2)
34-34: Nice use of modern Angular syntax!
The implementation uses the new output() function introduced in recent Angular versions, which is more concise than the traditional EventEmitter.
106-110: LGTM: Reactive form value changes handling
The implementation correctly uses takeUntil for proper cleanup and follows reactive programming patterns.
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts (2)
18-21: LGTM! Good use of Angular's new output syntax.
The component correctly implements the new output properties using Angular's modern syntax.
80-85: LGTM! Good test coverage for channel scope changes.
The test properly verifies the behavior of the course-wide channel toggle.
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.html (2)
86-90: LGTM! Good use of help text.
The help text implementation follows the established pattern and uses proper translation keys.
73-74: Review default selection behavior.
The checked attribute is set on the course-wide option (true), but the form control might initialize with false. This could lead to a mismatch between the visual state and the actual form value.
src/main/webapp/i18n/en/conversation.json (1)
211-211: LGTM! Clear and consistent translations.
The added translations for the course-wide channel feature are well-structured and provide clear explanations to users.
Also applies to: 240-245
src/test/playwright/support/pageobjects/course/CourseMessagesPage.ts (1)
115-120: LGTM! Well-structured implementation.
The new setCourseWideChannel method follows the established patterns:
- Consistent with other setter methods in the class
- Clear JSDoc documentation
- Uses consistent selector pattern
src/test/java/de/tum/cit/aet/artemis/communication/AbstractConversationTest.java (2)
183-183: LGTM: Setting default behavior for course-wide channels.
The addition of setIsCourseWide(false) ensures that channels are not course-wide by default, maintaining backward compatibility and allowing explicit opt-in for course-wide channels.
Line range hint 186-196: LGTM: Well-structured helper method for course-wide channels.
The new createCourseWideChannel method provides a clean, dedicated way to create course-wide channels in tests. The method sets appropriate defaults and follows existing patterns.
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (2)
Line range hint 65-77: LGTM: Well-structured sidebar visibility defaults.
The DEFAULT_SHOW_ALWAYS constant provides clear, logical defaults for channel visibility in the sidebar, following TypeScript naming conventions and covering all channel categories.
246-246: LGTM: Enhanced duplicate prevention in channel actions.
The addition of prev.channel.name === curr.channel.name to the distinctUntilChanged operator improves performance by preventing duplicate emissions when only the channel name changes.
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (2)
235-235: LGTM: Course-wide channel property is properly set
The implementation correctly sets the course-wide property from the DTO, maintaining consistency with other channel properties.
Line range hint 1020-1020: LGTM: Proper access control for course-wide channels
The implementation correctly allows access to course-wide channels for all course members, following security best practices.
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
937-937: LGTM: Feedback channels correctly set as non-course-wide
The tests properly verify that feedback channels are not course-wide by default, which is the expected behavior.
Also applies to: 962-962
...rse-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.spec.ts
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on Ts4, and works as expected. I agree with @sachmii here, that the # and announcement icon at the same time are a bit weird, but since this is unrelated to the change, and also happens on other testservers, genreally when creating announcement channels I'd propose to fix this in a follow up?
sawys777
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
dismissed stale reviews from HawKhiem, sawys777, SimonEntholzer, and coderabbitai[bot]
via
03ecb97
Since the "create channel" functionality is configured in the code to require at least tutor permissions, I set the "create channel" option to be invisible to students. As a result, the "create channel" dialog is already inaccessible to students, meaning they cannot open it. I tested this with test accounts for different roles: tutors, editors, and instructors can open the "create channel" dialog. Therefore, I didn’t need to add a role check specifically for the course-wide option. |
asliayk
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
requested review from
sachmii,
krusche,
sawys777,
SimonEntholzer and
HawKhiem
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Client
Motivation and Context
Communication: Support for Course-wide Group Chats #10012)Communication: Cannot create channels consecutively #10049)Description
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
create channel dialog with new option
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests