Communication: Add shortcut to private messages on usernames
#10007
Conversation
PaRangger
added
feature
server
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
…ion/add-shortcut-to-private-messages
PaRangger
temporarily deployed
to
artemis-test5.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/OneToOneChatResource.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/OneToOneChatIntegrationTest.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. WalkthroughThe pull request introduces several enhancements to the one-to-one chat functionality within the application. A new method for starting chats using user IDs has been added, alongside refactoring existing methods for better input validation and chat creation. The HTML components have been updated to improve user interactions, including new event bindings. Additionally, multiple test cases have been added or modified to ensure comprehensive coverage of the new functionalities and maintain existing logic integrity. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (12)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/OneToOneChatResource.java (1)
125-140: Consider moving business logic to the service layerWhile the extraction of
validateInputElseThrowandcreateOneToOneChatmethods improves organization, consider moving these methods to a service class to adhere to the single responsibility principle and keep the controller thin.src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (1)
23-27: Consider passing an empty object instead of null in POST requestIn the
createWithIdmethod, passing{}instead ofnullas the body of the POST request improves clarity and adheres to HttpClient conventions.Apply this diff to make the change:
return this.http - .post<OneToOneChatDTO>(`${this.resourceUrl}${courseId}/one-to-one-chats/${userIdOfChatPartner}`, null, { observe: 'response' }) + .post<OneToOneChatDTO>(`${this.resourceUrl}${courseId}/one-to-one-chats/${userIdOfChatPartner}`, {}, { observe: 'response' }) .pipe(map(this.conversationService.convertDateFromServer));src/test/javascript/spec/helpers/mocks/service/mock-metis-conversation.service.ts (1)
43-50: Ensure mock methods' return types match the actual serviceTo maintain consistency and type safety, consider specifying return types that match the actual
MetisConversationServicemethods, such asObservable<ConversationDTO>.Apply this diff to adjust the return types:
createOneToOneChatWithId = (userId: number): Observable<never> => { - return EMPTY; + return of(new ConversationDTO()); }; createOneToOneChat = (userId: number): Observable<never> => { - return EMPTY; + return of(new ConversationDTO()); };src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html (1)
18-18: Consider extracting common username click functionality.The implementation is correct but identical to answer-post-header.component.html. Consider extracting this into a shared component or directive to promote DRY principles.
You could create a shared directive:
@Directive({ selector: '[jhiClickableUsername]' }) export class ClickableUsernameDirective { @Input() isAuthorOfPosting: boolean; @Input() authorRole: any; @Output() userNameClicked = new EventEmitter<void>(); }Then use it in both components:
-<span class="fw-semibold" [ngClass]="{ clickable: !isAuthorOfPosting && posting.authorRole }" (click)="userNameClicked()"> +<span class="fw-semibold" jhiClickableUsername [isAuthorOfPosting]="isAuthorOfPosting" [authorRole]="posting.authorRole" (userNameClicked)="userNameClicked()">src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts (1)
105-111: Consider adding return type and improving error handling.While the implementation is correct, it could benefit from explicit typing and error handling.
- protected userNameClicked() { + protected userNameClicked(): void { if (this.isAuthorOfPosting || !this.posting.authorRole) { + console.debug('Ignoring click on own username or deleted user'); return; } this.onUserNameClicked.emit(); }src/main/webapp/app/shared/metis/posting.directive.ts (2)
35-39: Use private access modifier for injected services.The services should be marked as private since they're only used within the class.
- protected oneToOneChatService = inject(OneToOneChatService); - protected metisConversationService = inject(MetisConversationService); - protected metisService = inject(MetisService); - protected changeDetector = inject(ChangeDetectorRef); - protected router = inject(Router); + private readonly oneToOneChatService = inject(OneToOneChatService); + private readonly metisConversationService = inject(MetisConversationService); + private readonly metisService = inject(MetisService); + private readonly changeDetector = inject(ChangeDetectorRef); + private readonly router = inject(Router);
7-10: Consider organizing imports.Group related imports together for better readability.
import { Posting } from 'app/entities/metis/posting.model'; import { ChangeDetectorRef, Directive, Input, OnDestroy, OnInit, inject } from '@angular/core'; +import { Router } from '@angular/router'; + import { MetisService } from 'app/shared/metis/metis.service'; import { DisplayPriority } from 'app/shared/metis/metis.util'; -import { faBookmark } from '@fortawesome/free-solid-svg-icons'; -import { faBookmark as farBookmark } from '@fortawesome/free-regular-svg-icons'; import { isMessagingEnabled } from 'app/entities/course.model'; import { OneToOneChatService } from 'app/shared/metis/conversations/one-to-one-chat.service'; import { MetisConversationService } from 'app/shared/metis/metis-conversation.service'; -import { Router } from '@angular/router'; + +import { faBookmark } from '@fortawesome/free-solid-svg-icons'; +import { faBookmark as farBookmark } from '@fortawesome/free-regular-svg-icons';src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts (1)
272-296: Consider adding error case testingThe test suite would benefit from additional test cases that verify error handling scenarios, such as:
- Invalid user ID
- Network errors
- Server errors
Example test case:
it('should handle errors when creating one to one chat with id', () => { return new Promise((done) => { metisConversationService.setUpConversationService(course).subscribe({ complete: () => { const error = new HttpErrorResponse({ status: 404, statusText: 'Not Found' }); const createOneToOneChatSpy = jest.spyOn(oneToOneChatService, 'createWithId') .mockReturnValue(throwError(() => error)); const alertSpy = jest.spyOn(alertService, 'addAlert'); metisConversationService.createOneToOneChatWithId(999).subscribe({ error: () => { expect(createOneToOneChatSpy).toHaveBeenCalledWith(course.id, 999); expect(alertSpy).toHaveBeenCalled(); done({}); } }); } }); }); });src/main/webapp/app/shared/metis/metis.service.ts (4)
Line range hint
1-824: Consider splitting the service into smaller, focused services.The
MetisServicehas grown quite large and handles multiple responsibilities (posts, reactions, websockets, routing). This violates the Single Responsibility Principle and makes the code harder to maintain and test. Consider splitting it into:
MetisPostService: Handle post-related operationsMetisWebSocketService: Handle websocket communicationsMetisRoutingService: Handle routing logic
Line range hint
1-824: Prevent memory leaks by completing subjects and managing subscriptions.Several potential memory leak points were identified:
- The
posts$,tags$, andtotalNumberOfPosts$subjects are never completed- Some method subscriptions (e.g., in
deletePost,deleteAnswerPost) don't unsubscribeConsider implementing the following:
- Complete the subjects in ngOnDestroy:
ngOnDestroy(): void { if (this.subscriptionChannel) { this.jhiWebsocketService.unsubscribe(this.subscriptionChannel); } this.courseWideTopicSubscription.unsubscribe(); + this.posts$.complete(); + this.tags$.complete(); + this.totalNumberOfPosts$.complete(); }
- Use
takeUntilor store subscriptions for cleanup:deletePost(post: Post): void { - this.postService + const subscription = this.postService .delete(this.courseId, post) .pipe( tap(() => { // ... existing code ... }), ) .subscribe(); + this.subscriptions.push(subscription); }
Line range hint
1-824: Improve error handling across service methods.The service lacks comprehensive error handling for HTTP and WebSocket operations. This could lead to silent failures and poor user experience.
Consider implementing error handling:
- Add error handling to HTTP operations:
createPost(post: Post): Observable<Post> { return this.postService.create(this.courseId, post).pipe( map((res: HttpResponse<Post>) => res.body!), tap((createdPost: Post) => { // ... existing code ... }), + catchError(error => { + this.notificationService.error('Error creating post'); + return throwError(() => error); + }) ); }
- Add WebSocket error handling:
private handleNewOrUpdatedMessage = (postDTO: MetisPostDTO): void => { + try { // ... existing code ... + } catch (error) { + console.error('Error processing WebSocket message:', error); + this.notificationService.error('Error processing message'); + } };
Line range hint
1-824: Enhance security with consistent permission checks and input validation.Several security considerations need to be addressed:
- Ensure consistent permission checks before operations
- Add input validation for user-provided data
- Protect against XSS in post content
Consider implementing these security enhancements:
- Create a permission check decorator:
function requirePermission(minRole: 'TUTOR' | 'INSTRUCTOR') { return function (target: any, propertyKey: string, descriptor: PropertyDescriptor) { const originalMethod = descriptor.value; descriptor.value = function (...args: any[]) { if (minRole === 'INSTRUCTOR' && !this.metisUserIsAtLeastInstructorInCourse()) { throw new Error('Insufficient permissions'); } if (minRole === 'TUTOR' && !this.metisUserIsAtLeastTutorInCourse()) { throw new Error('Insufficient permissions'); } return originalMethod.apply(this, args); }; return descriptor; }; }
- Add input validation:
createPost(post: Post): Observable<Post> { + if (!post.content || post.content.trim().length === 0) { + return throwError(() => new Error('Post content is required')); + } return this.postService.create(this.courseId, post).pipe( // ... existing code ... ); }
- Sanitize post content:
createPost(post: Post): Observable<Post> { + post.content = this.sanitizer.sanitize(SecurityContext.HTML, post.content); return this.postService.create(this.courseId, post).pipe( // ... existing code ... ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/OneToOneChatResource.java(3 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(2 hunks)src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts(1 hunks)src/main/webapp/app/shared/metis/metis-conversation.service.ts(1 hunks)src/main/webapp/app/shared/metis/metis.service.ts(1 hunks)src/main/webapp/app/shared/metis/post/post.component.html(1 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts(3 hunks)src/main/webapp/app/shared/metis/posting.directive.ts(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/OneToOneChatIntegrationTest.java(6 hunks)src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/services/one-to-one-chat.service.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(1 hunks)src/test/javascript/spec/directive/posting.directive.spec.ts(3 hunks)src/test/javascript/spec/helpers/mocks/service/mock-metis-conversation.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
src/main/webapp/app/shared/metis/post/post.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/shared/metis/posting-header/post-header/post-header.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/shared/metis/posting-header/answer-post-header/answer-post-header.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/shared/metis/conversations/one-to-one-chat.service.ts (1)
src/test/javascript/spec/helpers/mocks/service/mock-metis-conversation.service.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/javascript/spec/component/shared/metis/post/post.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/shared/metis/metis.service.ts (1)
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.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/javascript/spec/component/shared/metis/answer-post/answer-post.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/shared/metis/answer-post/answer-post.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/shared/metis/posting-header/posting-header.directive.ts (1)
src/main/webapp/app/shared/metis/posting.directive.ts (1)
src/test/javascript/spec/component/overview/course-conversations/services/one-to-one-chat.service.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/javascript/spec/directive/posting.directive.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/java/de/tum/cit/aet/artemis/communication/web/conversation/OneToOneChatResource.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/test/java/de/tum/cit/aet/artemis/communication/OneToOneChatIntegrationTest.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 (25)
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (3)
21-23: LGTM: Import changes align with testing requirements
The new imports properly support HTTP testing, storage services, and conversation services needed for the username click feature. The organization follows the project's conventions.
Also applies to: 27-32
59-60: LGTM: Provider configuration follows testing best practices
The HTTP client and service mocks are properly configured following the project's testing guidelines.
Also applies to: 64-65
59-65: Add test coverage for username click functionality
Given that this component will now support clicking usernames to start private messages (per PR objectives), we should add test cases to verify:
- Username click event handling
- Integration with MetisConversationService
- Error scenarios (e.g., clicking own username, deleted users)
Let's verify if there are any existing tests for this functionality:
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/OneToOneChatResource.java (4)
28-29: Necessary imports added
The additions of Course and User imports are required for the new functionalities implemented.
81-81: Good practice of input validation
Refactoring the input validation into the validateInputElseThrow method enhances code readability and reusability.
95-96: Improved code maintainability by extracting chat creation logic
By moving the chat creation logic into the createOneToOneChat method, you've reduced code duplication and improved maintainability.
98-117: New endpoint for initiating chats using user IDs
The addition of the startOneToOneChat method with user ID enhances the API, providing a more efficient way to initiate chats.
src/test/javascript/spec/directive/posting.directive.spec.ts (4)
75-84: Proper use of mocks and providers in test setup
The inclusion of mock services and providers ensures that the tests are isolated and dependencies are correctly mocked.
177-184: Correct handling of undefined author in tests
The test effectively checks the scenario when author is undefined and uses the appropriate Jest matcher not.toHaveBeenCalled().
249-251: Using appropriate Jest matcher for boolean assertion
The test correctly uses toBeTrue() to assert boolean values, adhering to the testing guidelines.
301-310: Accurate implementation of timer decrement in tests
The use of jest.advanceTimersByTime() and checking the decremented value demonstrates precise control over timer functions in tests.
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html (1)
18-18: LGTM! Username click functionality implemented correctly.
The implementation correctly prevents clicking own username and handles user roles appropriately. The conditional class application aligns with the PR objectives.
Let's verify the CSS class implementation:
✅ Verification successful
CSS implementation verified, username click functionality is properly supported
The implementation is correct and well-supported:
- The
.clickableclass is properly defined inglobal.scsswithcursor: pointerstyling - The same click handler pattern is consistently used in both post header components
- The conditional class application prevents clicking own username and respects user roles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the clickable class definition exists in CSS files
# and check for any similar existing implementations
# Search for clickable class definition
rg -g '*.{css,scss}' 'clickable\s*{'
# Search for any similar click handlers on usernames
rg -g '*.html' 'userNameClicked|username.*click'
Length of output: 907
Script:
#!/bin/bash
# Fix the regex and search for clickable class definition
rg -g '*.{css,scss}' '\.clickable'
# Search for the component implementation to verify userNameClicked handler
ast-grep --pattern 'userNameClicked() {
$$$
}'
Length of output: 820
Script:
#!/bin/bash
# Search for the component implementation with correct TypeScript syntax
ast-grep --pattern 'class $_ {
$$$
userNameClicked() {
$$$
}
$$$
}'
# Check the clickable class definition in global.scss
cat src/main/webapp/content/scss/global.scss | grep -A 5 '\.clickable'
Length of output: 358
src/test/javascript/spec/component/overview/course-conversations/services/one-to-one-chat.service.spec.ts (1)
15-17: LGTM! Mock setup is clean and focused.
The ConversationService mock is properly implemented with the necessary convertDateFromServer method.
src/main/webapp/app/shared/metis/posting-header/posting-header.directive.ts (1)
22-22: LGTM! Modern Angular syntax used.
The output property uses the new-style Angular output syntax, which is more type-safe and performant.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (2)
27-27: LGTM! Event binding follows Angular conventions.
The event binding is correctly implemented and follows the component communication pattern.
43-43: LGTM! Event handler updated correctly.
The event handler has been properly updated to use the new method.
src/main/webapp/app/shared/metis/post/post.component.ts (1)
26-26: LGTM! Verify chat functionality relocation.
The removal of chat-related functionality and addition of the messaging check import aligns with single responsibility principle.
✅ Verification successful
The search results show that the chat functionality is still present and properly organized in the codebase:
OneToOneChatServiceis defined inconversations/one-to-one-chat.service.ts- The chat functionality is used through
PostingDirectivewhich is the base class for post components onUserReferenceClickedis still being used in both post and answer-post components- The functionality is properly integrated with
MetisConversationService
Let me verify the specific changes in the post component to ensure the refactoring was done correctly:
Let me try a different approach to verify the specific changes:
LGTM! Chat functionality is properly organized and maintained
The chat functionality has not been removed but rather is well-organized:
- The
PostComponentinherits fromPostingDirectivewhich contains the chat-related functionality - The
onUserReferenceClickedmethod is implemented in the base class and handles both communication page and regular page scenarios - The new import
isCommunicationEnabledis used to check if messaging is enabled before attempting to use chat features - The functionality is properly integrated with
OneToOneChatServiceandMetisConversationService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the chat functionality has been properly relocated
rg -A 5 "onUserReferenceClicked|OneToOneChatService" src/main/webapp/
Length of output: 6301
Script:
#!/bin/bash
# Check the implementation of post component and its parent class
ast-grep --pattern 'export class PostComponent {
$$$
}'
ast-grep --pattern 'export abstract class PostingDirective {
$$$
onUserReferenceClicked($_) {
$$$
}
$$$
}'
Length of output: 304
Script:
#!/bin/bash
# Check the specific changes in post component and its parent class
rg -l "class PostComponent" src/main/webapp/
rg -l "class PostingDirective" src/main/webapp/
# Then get their content
cat $(rg -l "class PostComponent" src/main/webapp/) || true
cat $(rg -l "class PostingDirective" src/main/webapp/) || true
Length of output: 16831
src/main/webapp/app/shared/metis/post/post.component.html (1)
27-27: LGTM! Verify event handler implementation.
The addition of onUserNameClicked event binding enables the core feature of clicking usernames to start chats.
✅ Verification successful
LGTM! Event handler is properly implemented and tested
The onUserNameClicked event handler is well implemented in the posting.directive.ts with proper checks and functionality:
- Guards against undefined author
- Checks if messaging is enabled
- Creates one-to-one chat based on the author ID
- Comprehensive test coverage including edge cases and success scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the event handler implementation
ast-grep --pattern 'onUserNameClicked($$$)' src/main/webapp/
Length of output: 62
Script:
#!/bin/bash
# Search for TypeScript/JavaScript files containing onUserNameClicked
rg -t ts -t js "onUserNameClicked" -A 5
# Search for the component class definition containing onUserNameClicked
ast-grep --pattern 'export class $_ {
$$$
onUserNameClicked($$$) {
$$$
}
$$$
}'
Length of output: 5796
src/test/java/de/tum/cit/aet/artemis/communication/OneToOneChatIntegrationTest.java (4)
Line range hint 52-149: LGTM! Test method names follow BDD conventions.
The renamed test methods clearly describe their behavior and expected outcomes, following the "should" pattern.
153-163: LGTM! Comprehensive test coverage for new chat creation.
Test properly verifies successful chat creation using user IDs and checks participant count.
165-179: LGTM! Edge case handling for existing chats.
Test ensures that attempting to create a chat with the same user returns the existing chat instead of creating a duplicate.
181-206: LGTM! Error cases are well covered.
Tests handle:
- Unknown user IDs (404)
- Disabled messaging (403)
- Unauthorized access (403)
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
96-96: LGTM: Proper test setup initialization
The addition of metisService.course = metisCourse in the beforeEach block ensures proper test context by initializing the required course information.
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
213-214: LGTM: Well-structured method addition
The new createOneToOneChatWithId method follows the established patterns:
- Consistent with existing chat creation methods
- Proper use of arrow function syntax
- Correct service delegation
src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts (1)
272-296: LGTM: Comprehensive test coverage for new method
The test case thoroughly verifies the createOneToOneChatWithId functionality:
- Proper service mocking
- Verification of method calls and parameters
- Assertion of expected outcomes
...script/spec/component/overview/course-conversations/services/one-to-one-chat.service.spec.ts
Show resolved
Hide resolved
ItsaaaMeMario
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Communication: Create shortcut to private messages by clicking usernames in channelCommunication: Add shortcut to private messages on usernames
krusche
deleted the
feature/communication/add-shortcut-to-private-messages
branch
Checklist
General
Server
Client
Motivation and Context
Currently you the only option for you to directly create a one to one chat with another user is by clicking the create channel icon and searching for the user. This creates a lot of interactions and server calls, which could be avoided by simply providing an option to click usernames to jump to a private channel.
Description
I added the option to click on usernames in channels to directly jump to a one to one chat with the user. You should not be able to click your own username or the username of deleted users. Additionally, I added a api to create a one to one chat by user id for the native apps.
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
Client
Server
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores